Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend support for <Package>_CXX_FLAGS to subpackages #442

Closed
bartlettroscoe opened this issue Jan 28, 2022 · 10 comments
Closed

Extend support for <Package>_CXX_FLAGS to subpackages #442

bartlettroscoe opened this issue Jan 28, 2022 · 10 comments

Comments

@bartlettroscoe
Copy link
Member

Currently, as described in:

one can set:

-D <TRIBITS_PACKAGE>_<LANG>_FLAGS="<EXTRA_COMPILER_OPTIONS>"

to append compiler flags at the package level. But there is currently no support for compiler options targeting the subpackage level. This was requested in trilinos/Trilinos#10111 for setting ShyLU_DDFROSch_CXX_FLAGS for the ShyLU_DDFROSch subpackages in the ShyLU_DD package.

Possible implementations

This could be implemented in one of two options.

Option-1: One approach would be for <SubPackage>_<LANG>_FLAGS to replace <ParentPackage>_<LANG>_FLAGS flags such that setting:

  -D ShyLU_DD_<LANG>_FLAGS="<flags-1>" \
  -D ShyLU_DDFROSch_<LANG>_FLAGS="<flags-2>" \

would result in the flags <flags-2> only being appended to the CMAKE_CXX_FLAGS and for <flags-1> to be ignored for the ShyLU_DDFROSch subpackage while <flags-1> would be appended for the other subpackages in the parent package ShyLU_DD.

option-2: The other approach would be for the <SubPackage>_<LANG>_FLAGS to append those in <ParentPackage>_<LANG>_FLAGS so that:

  -D ShyLU_DD_<LANG>_FLAGS="<flags-1>" \
  -D ShyLU_DDFROSch_<LANG>_FLAGS="<flags-2>" \

resulted in the flags <flags-1> <flags-2> being appended to the subpackage ShyLU_DDFROSch CMAKE_CXX_FLAGS while <flags-1> while <flags-1> would be appended for the other subpackages in the parent package ShyLU_DD.

@bartlettroscoe
Copy link
Member Author

@searhein, should we go with option-1 or option-2 above? That is really the only thing that needs to be decided here before I begin.

@searhein
Copy link

@bartlettroscoe Thank you for looking into this.

Honestly, there is no connection/dependency between FROSch and other sub-packages in ShyLU_DD. In that sense, both options would be exactly the same for me: in practice, I would just write all flags in ShyLU_DDFROSch_<LANG>_FLAGS.

Somehow, option 2 feels a bit more intuitive to me, but for ShyLU_DD it would not make much of a difference.

@bartlettroscoe
Copy link
Member Author

Somehow, option 2 feels a bit more intuitive to me, but for ShyLU_DD it would not make much of a difference.

@searhein, that was my opinion as well but I wanted to get an unbiased second opinion. I will given that a try.

@bartlettroscoe
Copy link
Member Author

Honestly, there is no connection/dependency between FROSch and other sub-packages in ShyLU_DD

@searhein, then FROSch likely should have been its own top-level package. Generally, subpackages should be aggregated into a parent package only if they are related in some obvious way.

@searhein
Copy link

@bartlettroscoe Thank you!

I agree on that. At the point when I integrated FROSch, the idea was to gather all the DD work in ShyLU_DD. However, I agree that it would also make much sense to have FROSch as a top-level package; for instance in this case, this would make things easier. Maybe, this is something to reconsider?

@bartlettroscoe
Copy link
Member Author

@searhein,

At the point when I integrated FROSch, the idea was to gather all the DD work in ShyLU_DD

Well, that is a relationship then (even if it is not a software relationship).

Maybe, this is something to reconsider?

That is something you should discuss with Siva and the other Trilinos leads at some point. I think there is some desire to actually reduce the number of top-level Trilinos packages. But no decision on that should be made, in my opinion, until we complete #367 and some related follow-ons. Breaking Trilinos into different repos (with multiple packages per repo) might make more sense. But it is not my place to speculate too much on this.

@searhein
Copy link

@bartlettroscoe I fully agree. Being able to specify the flags for FROSch individually would already help me a lot. Thanks!

@bartlettroscoe bartlettroscoe moved this from Selected to In Progress in TriBITS Feb 18, 2022
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 19, 2022
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 19, 2022
As part of this I also:

* Refactored the code some to be more maintainable

NOTE: The tests I added are very strong but may not be 100% portable.  The
should only run with Linux, wtih Makefiles and with GCC and therefore should
be okay.  We only need to test this logic with one CMake generator and with
compiler.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 19, 2022
As part of this I also:

* Refactored the code some to be more maintainable

* Added documentation for subpackages-specific compiler flags

* Added a new doc subsetion to explain how to print out compiler options for
  each package

* Moved the doc subsection for adding arbitrary link options after all of the
  subsections about manipulating compiler flags/options
bartlettroscoe added a commit that referenced this issue Feb 19, 2022
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 19, 2022
As part of this I also:

* Refactored the code some to be more maintainable

* Added documentation for subpackages-specific compiler flags

* Added a new doc subsetion to explain how to print out compiler options for
  each package

* Moved the doc subsection for adding arbitrary link options after all of the
  subsections about manipulating compiler flags/options
@bartlettroscoe
Copy link
Member Author

@searhein,

This is done in TriBITS 'master' as of PR #453 that I just merged. Can you please review the updated documentation described at the top of PR #453 and make comments there?

I will next do the git gymnastics to cherry-pick and then snapshot back just this one commit to a branch off Trilinos 'develop'. (I can't snapshot all of TriBITS to Trilinos due to #433).

@bartlettroscoe bartlettroscoe moved this from In Progress to In Review in TriBITS Feb 19, 2022
@bartlettroscoe
Copy link
Member Author

FYI: The Trilinos PR that brings just this PR back to the version in Trilinos 'develop' is trilinos/Trilinos#10219.

@bartlettroscoe
Copy link
Member Author

This has been done for a long time with the merge of PRs #453 and trilinos/Trilinos#10219.

Closing as complete.

TriBITS automation moved this from In Review to Done Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
TriBITS
  
Done
Development

No branches or pull requests

2 participants