[cmake-developers] target_include_directories branch in stage

David Cole david.cole at kitware.com
Mon Jan 30 08:12:12 EST 2012


On Sun, Jan 29, 2012 at 11:44 AM, Stephen Kelly <steveire at gmail.com> wrote:
> David Cole wrote:
>> OK... nearly complete now. Please review, then reply and tell me if
>> you object to any of the 7 commits in this topic branch.
>
> No objections. They all seem fine.
>

Great, thanks.


>> Steve, I've
>> preserved your authorship for most of these commits, but have
>> significantly re-written some of them.
>
> The 3 following 073bc421620e25fef6389b0d8b71cfad8ca79786 (CMake: Eliminate
> cmMakefile::IncludeDirectories) seem to have been substantially re-written.
> I'm not sure what is the appropriate way to deal with commits like that, but
> whatever you choose for the author line is fine.
>

So I guess the question is: if somebody looks up the author at some
point in the future and comes asking a question, do you want to be the
one they go to, or would you rather have them come to me? ;-) If you
want to send them to me, I'll change the author line, but you
certainly deserve the credit for getting the ball rolling on this
topic.


>>
>> I've pushed again (force-pushed, please delete previous checkouts of
>> this branch) to github:
>>
>>   https://github.com/dlrdave/CMake/commits/tgt-inc-dirs-vs-xcode
>>   https://github.com/dlrdave/CMake/commit/0a45e239
>>
>> I've verified all tests pass on Mac with Unix Makefiles, Mac with
>> Xcode, Windows with VS10 and Windows with VS6. I'm confident we will
>> have minimal dashboard issues when merging this to 'next'.
>
> Great. Thanks for all of your efforts.
>
>> What remains to be done:
>>   - need ExpandVariablesInString calls for the target properties, too...
>>   at
>>       the same point, or is there a better time to call it?
>
> Are you asking whether cmMakefile::ExpandVariables() should do something
> like (sort-of pseudocode):
>
> foreach(Target target, this->Targets)
>  {
>    const char *includeDirs = target->GetProperty("INCLUDE_DIRECTORIES");
>    if (includeDirs)
>      {
>      std::string dirs = includeDirs;
>      this->ExpandVariablesInString(dirs, true, true);
>      target->SetProperty("INCLUDE_DIRECTORIES", dirs.c_str());
>      }
>  }
>
> I don't know enough about when ExpandVariables is supposed to be called etc,
> but it seems like a sensible place to do it is always called at the correct
> point.
>
> However, it starts to look like ExpandVariablesInString should be API
> elsewhere in CMake (cmSystemTools). It's not a very important point though
> and doesn't necessarily need to be solved with this branch.
>

I'll talk this over with Brad and finalize the commits with any
changes before pushing this to 'next'.


>>   - avoid duplicates in "cmMakeDepend::SetMakefile"?
>
> Considering that it used to call
>
>  this->Makefile->GetIncludeDirectories();
>
> which does preserve uniqueness, I think it makes sense to do so with the
> ported code too.
>

Yes, ok, I'll spend the extra effort to make that section a unique list as well.


>>   - double-check with Alex about the changes in cmake::FindPackage --
>>   Alex?
>
>>   - fully remove cmMakefile::GetIncludeDirectories since it has no
>>   more callers - update documentation for include_directories and the
>> INCLUDE_DIRECTORIES property
>
> I have updated my branch on gitorious with an attempt at documenting the
> change (commit 8acd1c07ff299ea823837ba6268b43db92ac81f2).
>

Thanks -- I'll pull this and use it as a basis for the final topic.


David



More information about the cmake-developers mailing list