Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 05/02/2013 01:14 PM, Brad King wrote: > On 05/02/2013 10:57 AM, Stephen Kelly wrote: >> I've added a patch to fix-per-config-tll-include-dirs in my clone. Is that >> what you're looking for? > > Yes. Thanks. I've rewritten that topic history to organize the net fixes into three commits: Centralize maintenance of usage requirement include directories http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=b8259c3d Fix include dir propagation from conditionally linked targets http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=26dba6a1 Memoize usage requirement include directories in a config-specific map http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=dea1df4e Please extend the topic with tests for the problematic case(s) that exposed this. Thanks, -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 05/02/2013 10:57 AM, Stephen Kelly wrote: > I've added a patch to fix-per-config-tll-include-dirs in my clone. Is that > what you're looking for? Yes. Thanks, -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote: > On 05/02/2013 02:46 AM, Stephen Kelly wrote: >> Brad King wrote: >>> On 04/30/2013 07:38 PM, Stephen Kelly wrote: >>> I'm saying that even after the current version of the topic the >>> caching is still accumulating across configurations in multi-config >>> generators. AFAICT that part is not yet fixed. The >>> CachedLinkInterfaceIncludeDirectoriesEntries needs to map from config >>> to list of entries. I've added a patch to fix-per-config-tll-include-dirs in my clone. Is that what you're looking for? If not it might be better to produce the patch and I can review it. 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 05/02/2013 02:46 AM, Stephen Kelly wrote:
> Brad King wrote:
>> On 04/30/2013 07:38 PM, Stephen Kelly wrote:
>> I'm saying that even after the current version of the topic the
>> caching is still accumulating across configurations in multi-config
>> generators. AFAICT that part is not yet fixed. The
>> CachedLinkInterfaceIncludeDirectoriesEntries needs to map from config
>> to list of entries.
>
> Hmm, I don't think that is true currently.
The logic is:
if(cachingDone[config])
{
cachedEntries.push_back(expr);
}
cachingDone[config] = true;
When that runs for multiple configurations then it will duplicate the
expr entry, accumulating one for each config.
-Brad
--
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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote: > On 04/30/2013 07:38 PM, Stephen Kelly wrote: >> Brad King wrote: >>> Reading the commit messages and code in more detail, it looks wrong >>> to have CachedLinkInterfaceIncludeDirectoriesEntries not per-config. >>> It looks like the generated value can be different per-config and >>> currently it accumulates cached values across multiple configs. >> >> Yes, that's probably a more-accurate description. > > I'm saying that even after the current version of the topic the > caching is still accumulating across configurations in multi-config > generators. AFAICT that part is not yet fixed. The > CachedLinkInterfaceIncludeDirectoriesEntries needs to map from config > to list of entries. Hmm, I don't think that is true currently. Have you tried producing buggy code for the issue? The CachedLinkInterfaceIncludeDirectoriesEntries is a vector of IncludeDirectoriesEntry*, which contains the TargetName (doesn't change per- config), a cmCompiledGeneratorExpression (doesn't change per config), and a vector of cached includes. That includes cache is only populated if the includes are config-independent. I'm not sure if storing a map of config to IncludeDirectoriesEntry* instead would have an advantage. Probably only in projects with a large number of config-sensitive include directories. 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 04/30/2013 07:38 PM, Stephen Kelly wrote: > Brad King wrote: >> Reading the commit messages and code in more detail, it looks wrong >> to have CachedLinkInterfaceIncludeDirectoriesEntries not per-config. >> It looks like the generated value can be different per-config and >> currently it accumulates cached values across multiple configs. > > Yes, that's probably a more-accurate description. I'm saying that even after the current version of the topic the caching is still accumulating across configurations in multi-config generators. AFAICT that part is not yet fixed. The CachedLinkInterfaceIncludeDirectoriesEntries needs to map from config to list of entries. > Ah, I think I misread this. I just merged the commit to next anyway, so any > rebase will include that commit anyway Okay, no problem. -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote: > On 04/30/2013 04:55 AM, Stephen Kelly wrote: >> Brad King wrote: >>> Okay, that looks good. Please rebase on the partial fix I merged last >>> week so we can test it in 'next'. I'll squash all that together later. >> >> Ok, done. > > Reading the commit messages and code in more detail, it looks wrong > to have CachedLinkInterfaceIncludeDirectoriesEntries not per-config. > It looks like the generated value can be different per-config and > currently it accumulates cached values across multiple configs. Yes, that's probably a more-accurate description. > > CachedLinkInterfaceCompileDefinitions is per-config already. > >> I think that would be more complex than just fixing the issue. I've >> pushed an additional patch to fix it, but haven't merged it to next. > > Let's rebase that on the final version of the above fixes. Ah, I think I misread this. I just merged the commit to next anyway, so any rebase will include that commit anyway (I'm not sure what rebase you have in mind). 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 04/30/2013 04:55 AM, Stephen Kelly wrote: > Brad King wrote: >> Okay, that looks good. Please rebase on the partial fix I merged last >> week so we can test it in 'next'. I'll squash all that together later. > > Ok, done. Reading the commit messages and code in more detail, it looks wrong to have CachedLinkInterfaceIncludeDirectoriesEntries not per-config. It looks like the generated value can be different per-config and currently it accumulates cached values across multiple configs. CachedLinkInterfaceCompileDefinitions is per-config already. > I think that would be more complex than just fixing the issue. I've pushed > an additional patch to fix it, but haven't merged it to next. Let's rebase that on the final version of the above fixes. Either way later I'd like to not duplicate the tll() list for linking and includes with only the latter having context. They should both be folded into one list with context. -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote: > On 04/29/2013 06:29 AM, Stephen Kelly wrote: >> I've force-pushed the fix-multi-config-tll-include-dirs branch with a >> more- simple fix for this issue to my clone. > > Okay, that looks good. Please rebase on the partial fix I merged last > week so we can test it in 'next'. I'll squash all that together later. Ok, done. > > Also I realized my local topic using GetLinkInformation is wrong because > it includes usage requirements from everything linked, not just from > the link *interface* closure. I'll drop that one in favor of yours. > > While working through all of the above I realized that AppendTllIncludes > and LinkInterfaceIncludeDirectoriesEntries are not safe. Users can > re-write the LINK_LIBRARIES property and those other structures will > not be updated. IIUC they only exist to provide a backtrace for > include directory entries. Please drop them for now at the expense of > the backtraces which can be restored later by tracking the backtraces > of all tll() link information (similar to tid() now). > I think that would be more complex than just fixing the issue. I've pushed an additional patch to fix it, but haven't merged it to next. 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 04/29/2013 06:29 AM, Stephen Kelly wrote: > I've force-pushed the fix-multi-config-tll-include-dirs branch with a more- > simple fix for this issue to my clone. Okay, that looks good. Please rebase on the partial fix I merged last week so we can test it in 'next'. I'll squash all that together later. Also I realized my local topic using GetLinkInformation is wrong because it includes usage requirements from everything linked, not just from the link *interface* closure. I'll drop that one in favor of yours. While working through all of the above I realized that AppendTllIncludes and LinkInterfaceIncludeDirectoriesEntries are not safe. Users can re-write the LINK_LIBRARIES property and those other structures will not be updated. IIUC they only exist to provide a backtrace for include directory entries. Please drop them for now at the expense of the backtraces which can be restored later by tracking the backtraces of all tll() link information (similar to tid() now). Thanks, -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Stephen Kelly wrote: > It can probably be refactored to bypass the genex evaluation though. The refactoring I described can be done in the future I think, but doesn't really solve the bug anyway, because it is evaluation of the generator expression for the target itself which is causing problems here. I've force-pushed the fix-multi-config-tll-include-dirs branch with a more- simple fix for this issue to my clone. 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote: > The caching is simplified because we never need to clear it since these > are now only called during generation and never during configuration. I've looked again at the code, and there is a condition for clearing the cache if not configuring. I think this is because of export() at configure- time needing to call that code, but the cached values from those early evaluations wouldn't be appropriate for generation-time. 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote:
> On Fri, Apr 26, 2013 at 10:39 AM, Stephen Kelly
> wrote:
>> Brad King writes:
>>
>>> I looked into implementing this and realized that we're not propagating
>>> usage requirements from the entire link closure, only from the direct
>>> dependencies.
>>
>> Are you sure? Maybe I don't understand what you mean. Can you post a code
>> snippet?
>>
>> add_library(foo SHARED empty.cpp)
>> target_include_directories(foo INTERFACE /opt/foo)
>> add_library(bar SHARED empty.cpp)
>> add_library(sho SHARED empty.cpp)
>>
>> target_link_libraries(sho bar)
>> target_link_libraries(bar foo)
>
> It appears to work but I do not see how from a quick read of the
> implementation.
> The AppendTllIncludes is only called for direct dependencies by tll().
> What propagates the requirement transitively in 2.8.11-rc3?
Ah, indeed. This solves the mystery of why the current implementation
creates the generator expression for each entry instead of reading the
property directly.
$
gives a transitively evaluated result, but
fooTarget->GetDefinition("INTERFACE_INCLUDE_DIRECTORIES")
does not.
The former works because it is part of the targetPropertyTransitiveWhitelist
in cmGeneratorExpressionEvaluator when evaluating the TargetPropertyNode, so
I probably also wanted to ensure re-use of the logic around there for
context-sensitive conditions which relate to caching.
It can probably be refactored to bypass the genex evaluation though.
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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On Fri, Apr 26, 2013 at 10:39 AM, Stephen Kelly wrote: > Brad King writes: > >> I looked into implementing this and realized that we're not propagating >> usage requirements from the entire link closure, only from the direct >> dependencies. > > Are you sure? Maybe I don't understand what you mean. Can you post a code > snippet? > > add_library(foo SHARED empty.cpp) > target_include_directories(foo INTERFACE /opt/foo) > add_library(bar SHARED empty.cpp) > add_library(sho SHARED empty.cpp) > > target_link_libraries(sho bar) > target_link_libraries(bar foo) It appears to work but I do not see how from a quick read of the implementation. The AppendTllIncludes is only called for direct dependencies by tll(). What propagates the requirement transitively in 2.8.11-rc3? Thanks, -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King writes: > I looked into implementing this and realized that we're not propagating > usage requirements from the entire link closure, only from the direct > dependencies. Are you sure? Maybe I don't understand what you mean. Can you post a code snippet? add_library(foo SHARED empty.cpp) target_include_directories(foo INTERFACE /opt/foo) add_library(bar SHARED empty.cpp) add_library(sho SHARED empty.cpp) target_link_libraries(sho bar) target_link_libraries(bar foo) $ cat CMakeFiles/sho.dir/flags.make # CMAKE generated file: DO NOT EDIT! # Generated by "Unix Makefiles" Generator, CMake Version 2.8 # compile CXX with /usr/lib/icecc/bin/c++ CXX_FLAGS = -fPIC -I/opt/foo CXX_DEFINES = -Dsho_EXPORTS 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 4/25/2013 6:15 PM, Stephen Kelly wrote: > Brad King wrote: >> Why can't GetIncludeDirectories just evaluate it->Value as a >> genex with the current configuration, and if it is a valid target >> name then lookup its INTERFACE_INCLUDE_DIRECTORIES and use that >> immediately? > > Good question. Reading the code, I can't see any reason it needs to be that > way. I looked into implementing this and realized that we're not propagating usage requirements from the entire link closure, only from the direct dependencies. In an earlier design iteration the transitive requirements were brought in by evaluation of the interface properties themselves. Now that tll() doesn't touch the properties and the requirements are simply appended during generation we need to use the full link closure. I've drafted a change to convert cmTarget::GetIncludeDirectories and cmTarget::GetCompileDefinitions over to share a common internal usage requirements API that uses GetLinkInformation to get the transitive link closure and looks up the INTERFACE_* properties in every linked target. The caching is simplified because we never need to clear it since these are now only called during generation and never during configuration. I'll report back after doing more testing. Thanks, -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
Brad King wrote: >> I've pushed the fix-multi-config-tll-include-dirs branch to my clone. > > That's a much more detailed explanation, thanks. > > Pre-evaluating the expression as "Debug" regardless of the > configuration involved feels like a hack. Since this all occurs > at generate time already anyway, why are we constructing a genex > only to evaluate it immediately elsewhere in the generator code? > > Why can't GetIncludeDirectories just evaluate it->Value as a > genex with the current configuration, and if it is a valid target > name then lookup its INTERFACE_INCLUDE_DIRECTORIES and use that > immediately? Good question. Reading the code, I can't see any reason it needs to be that way. It's possible that when I implemented that I was focussed on re-using the code in the processIncludeDirectories function. Changing it as you describe would probably make the caching easier too because the includes could be cached directly. This is an area where we had performance problems before when running cmake on the llvm repo. 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 04/25/2013 11:02 AM, Stephen Kelly wrote: > Brad King wrote: >> Great! I've turned that patch into this commit: >> >> http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=de9d4a63 >> >> See commit message for an explanation that makes sense to me. > > The commit message is correct, but it's not the whole story. We also need to > validate the it->Value as a target name acceptable to TARGET_PROPERTY. > > I've pushed the fix-multi-config-tll-include-dirs branch to my clone. That's a much more detailed explanation, thanks. Pre-evaluating the expression as "Debug" regardless of the configuration involved feels like a hack. Since this all occurs at generate time already anyway, why are we constructing a genex only to evaluate it immediately elsewhere in the generator code? Why can't GetIncludeDirectories just evaluate it->Value as a genex with the current configuration, and if it is a valid target name then lookup its INTERFACE_INCLUDE_DIRECTORIES and use that immediately? Thanks, -Brad -- 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
Re: [cmake-developers] [CMake] 2.8.11-rc3 generator expression error
On 04/25/2013 03:34 AM, Stephen Kelly wrote: > I haven't had time to investigate fully, but this patch should 'fix' the > problem: [snip] > I'll investigate later to see if it's the right fix and why. Great! I've turned that patch into this commit: http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=de9d4a63 See commit message for an explanation that makes sense to me. Since these generator expressions are constructed and evaluated internally to CMake and never exposed it should be easy to adjust how this works in the future. Therefore I'm going to take this as the fix for 2.8.11-rc4 if it is clean on the test dashboard tonight. When you have time to investigate in more detail, please extend this topic with a test case covering target_link_libraries(B debug A) where A is a target instead of an external library name. Amazingly/apparently we do not have this in the test suite yet. Thanks, -Brad -- 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
