Brad King wrote:
> On 01/31/2013 10:23 AM, Stephen Kelly wrote:
>> I left it as $<LINKED> so far so that it is a shorter string, and because
>> the implementation is somewhat simpler.
>
> Ahh, I see you were using $<LINKED:...> even for already-defined targets.
> I think that is fine during processing of the same project to keep the
> strings shorter. We still need to keep it out of the exported interface
> so that we can change it later without having to keep the expression
> implementation around to support old export files. This way it never
> outlives the CMake process that generated it.
Fair enough. I've squashed that patch into the commit that introduces the
LINKED genex.
>
>> Do you think it needs to be done before merging to next?
>
> De-duplication of $<LINKED> references can be added later but I'd still
> like to see it before the 2.8.11 release so that it doesn't become a
> behavior change later.
Ok, I can look into that.
>
>> Anything else to do before merging it?
>
> Please add a test case for the $<INSTALL_PREFIX> evaluation error.
>
Done.
>
> What is the purpose of the else case here?
>
> + std::string value = !isGenex ? "$<LINKED:" + lib + ">"
> + : "$<$<TARGET_DEFINED:" + lib + ">:" +
> + "$<TARGET_PROPERTY:" + lib +
> + ",INTERFACE_" + property + ">"
> + ">";
>
> If the input is already a genex isn't the author responsible for
> making sure it is valid?
Yes, a valid genex for linking. Consider this:
add_library(foo ...)
add_library(bar ...)
target_link_libraries(foo $<$<CONFIG:Debug>:bar>)
The genex for the INCLUDE_DIRECTORIES of foo needs to be (excuse the python-
style concats):
"$<$<TARGET_DEFINED:" + "$<$<CONFIG:Debug>:bar>" + ">:" +
"$<TARGET_PROPERTY:" + "$<$<CONFIG:Debug>:bar>" +
",INTERFACE_" + "INCLUDE_DIRECTORIES" + ">"
">"
because the result of the genex could either be "" or "bar".
> The documentation of <package>_NO_INTERFACES and <package>_INTERFACES
> reference ${CMAKE_FIND_PACKAGE_NAME}_FIND_VERSION but the corresponding
> <package>_FIND_VERSION isn't documented until further down. Please move
> the new docs to the bottom, but still above the NO_POLICY_SCOPE line.
>
> Also the current wording of the documentation makes it sound like 2.8.11
> is at fault for introducing incompatibility. There is no need to mention
> the version of CMake there. It is the version of the upstream that
> starts using the new features that causes a problem.
>
> It should be clear that the upstream is responsible for adding the example
> code to set the <package>_NO_INTERFACES variable in their package config
> file when it exposes the new interfaces. Also <package>_INTERFACES is to
> be set by the downstream and used by that code in the upstream.
Ok. Hopefully that's more clear now.
>
>
> Then merge for testing!
Done.
Thanks,
Steve.
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
