Skip to content

Conversation

@dberzano
Copy link
Contributor

@dennisklein
Copy link
Member

@dberzano I have no objections here, but just to give a headsup what will come soon: In #632 I already changed the behaviour of all find modules to also search in system paths. Also I introduced a CMake variable matching the exported variable in your alidist recipes. So, in this case instead of ProtoBuf_DIR I used PROTOBUF_ROOT.

@MohammadAlTurany MohammadAlTurany merged commit f289bb1 into FairRootGroup:dev Oct 12, 2017
@dberzano
Copy link
Contributor Author

Thanks @dennisklein I saw your PR. Just to be clear:

CMake's de facto convention (i.e. for most FindPackageNames) is to pass PackageName_DIR variables, not PACKAGENAME_ROOT ones, and to rely on what you explicitly pass (e.g. with -D) as opposed to picking things from the environment.

What I think is that:

  • ProtoBuf_DIR and all other PackageName_DIR should be kept as they are and not deprecated in favour of PACKAGENAME_ROOT ones.
  • No PACKAGENAME_ROOT variable should be picked from the environment when configuring a project (following the principle of "explicit is better than implicit").

My two cents :-) I'll keep an eye on #632!

@dennisklein
Copy link
Member

(...) as opposed to picking things from the environment.

No PACKAGENAME_ROOT variable should be picked from the environment when configuring a project (following the principle of "explicit is better than implicit").

I fully agree with the notion, but it might not be possible to support all the needs of our clients by not picking up dependency hints from the environment. I will try to make everyone aware of this issue again and we will see, if I can convince them to support your requirement.

CMake's de facto convention (i.e. for most FindPackageNames) is to pass PackageName_DIR variables

I was not aware of this convention, thx. I see no reason not to support both variable naming schemes. If people agree to waive environment pickups, I would probably drop the aliBuild variable naming scheme again from #632 and stick with the CMake convention only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants