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

Set Trilinos_MUST_FIND_ALL_TPL_LIBS=TRUE by default for Trilinos 15.0 #10345

Closed
bartlettroscoe opened this issue Mar 17, 2022 · 18 comments
Closed
Assignees
Labels
PA: Framework Issues that fall under the Trilinos Framework Product Area TriBITS Issues with the TriBITS framework itself, not usage of the TriBITS framework type: enhancement Issue is an enhancement, not a bug
Milestone

Comments

@bartlettroscoe
Copy link
Member

@trilinos/framework

With Trilinos 14.0 coming up, this would be a good time to change the default for Trilinos_MUST_FIND_ALL_TPL_LIBS from OFF to ON. (By setting Trilinos_MUST_FIND_ALL_TPL_LIBS_DEFAULT to ON as described here). It was turned off by default because it breaks backward compatibility but it causes problems for new users and new configurations (see, for example, #10322 (comment)).

So as soon as the Trilinos 13.4 branch is created, we should make this change in the 'develop' branch.

@bartlettroscoe bartlettroscoe added PA: Framework Issues that fall under the Trilinos Framework Product Area type: enhancement Issue is an enhancement, not a bug TriBITS Issues with the TriBITS framework itself, not usage of the TriBITS framework labels Mar 17, 2022
@bartlettroscoe
Copy link
Member Author

@ccober6, would it be possible to create GitHub Issue Milestone for Trilinos 14.0 so that we can place this and other issues like it to remind us to do these things for Trilinos 14.0?

@github-actions
Copy link

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

@github-actions github-actions bot added the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Mar 18, 2023
@jwillenbring
Copy link
Member

Was this done, or is this still of interest?

@ccober6
Copy link
Contributor

ccober6 commented Mar 20, 2023

Hmm... We may have just missed this. @sebrowne

@sebrowne
Copy link
Contributor

sebrowne commented Mar 20, 2023

I missed it for sure. We can add it to the 14.0.1 release (I think that @hkthorn said that Xyce needs one, see #11685 )

Do we want it added to the 14.0 release, or just change it on develop so that it occurs in post-14.0 releases?

@ccober6
Copy link
Contributor

ccober6 commented Mar 20, 2023

I am in favor of getting it in 14.0.1 and develop, so long as it doesn't cause stakeholders a headache. They have not had any notice of this. If it causes backward incompatibilities we will need to wait until Trilinos 15.0 (~Sept 2023).

@bartlettroscoe
Copy link
Member Author

This will definitely break backward compatibility but likely in a good way 90% of the time. And users that don't want this behavior can just set -D Trilinos_MUST_FIND_ALL_TPL_LIBS=OFF (which is backward compatible).

Also, if I had realized that the Trilinos 14.0 release was immanent, I would have made sure and merge:

because that might technically break backward compatibility in some corner cases.

@bartlettroscoe
Copy link
Member Author

FYI, the conversation about backward compatibility seems to have evolved over the last few years. Now semver seems to differentiate "major" and "minor" breaks in backward compatibility and incrementing the major version number. For example, see:

If a single configure option can restore backward compatibility and if the change is backward compatibility is obvious, then I think that should not warrant an increment of the major version number.

In order words, I think changing the default of Trilinos_MUST_FIND_ALL_TPL_LIBS from OFF to ON should be okay in a patch release (also since most customers will not touch an X.Y.0 release and will wait for the X.Y.1 release before trying to use it).

Up to this point, very good backward compatibility is maintained for customers my setting a few configure options.

@ccober6
Copy link
Contributor

ccober6 commented Mar 20, 2023

@bartlettroscoe if you are comfortable with us patching 14.0 and merging to develop, I would say let's give it try and if we run into issues we can back it out.

@sebrowne, we should create a Trilinos 15.0 issue and link issues related to the release to help include issues like this with 15.

@github-actions github-actions bot removed the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label Mar 22, 2023
@bartlettroscoe bartlettroscoe changed the title Set Trilinos_MUST_FIND_ALL_TPL_LIBS=TRUE by default for Trilinos 14.0 Set Trilinos_MUST_FIND_ALL_TPL_LIBS=TRUE by default for Trilinos 15.0 Aug 3, 2023
@bartlettroscoe
Copy link
Member Author

@sebrowne, @ccober6, let's not forget to do this after Trilinos 14.4 is branched!

@bartlettroscoe bartlettroscoe added this to the trilinos-15.0 milestone Aug 3, 2023
@bartlettroscoe
Copy link
Member Author

@sebrowne and @ccober6, I created the GitHub Issue milestone trilinos-15.0 to help remind people of what needs to be done before the Trilinos 15.0 release.

@sebrowne
Copy link
Contributor

sebrowne commented Aug 7, 2023

Thanks Ross! I'm all in favor of putting this in ASAP after we get 14.4 out the door (should be very soon).

@sebrowne
Copy link
Contributor

@bartlettroscoe do you want me to do this now that 14.4 is out?

@bartlettroscoe
Copy link
Member Author

@bartlettroscoe do you want me to do this now that 14.4 is out?

Yes, but we will need to write a release note for this. And this may break some customer's configure scripts because they may have had extra libs they did not need that did not get found but it happened to work anyway.

@srbdev srbdev self-assigned this Sep 6, 2023
@srbdev
Copy link
Contributor

srbdev commented Sep 6, 2023

@bartlettroscoe should I make the change under cmake/tribits/core/package_arch/TribitsGlobalMacros.cmake on line 568 or should it only be made specific to Trilinos, with the Trilinos_ prefix?

@bartlettroscoe
Copy link
Member Author

@bartlettroscoe should I make the change under cmake/tribits/core/package_arch/TribitsGlobalMacros.cmake on line 568 or should it only be made specific to Trilinos, with the Trilinos_ prefix?

No, see:

Setting this in the CMakeLists.txt file makes more sense since this does not impact the dependency logic.

fryeguy52 added a commit to fryeguy52/Trilinos that referenced this issue Sep 28, 2023
setting Trilinos_MUST_FIND_ALL_TPL_LIBS_DEFAULT to on
@bartlettroscoe
Copy link
Member Author

With the merging of #12258, this should be complete. (But someone should have done a local build to ensure this is working to catch non-found libraries.)

@bartlettroscoe
Copy link
Member Author

This was completed with the merge of PR:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PA: Framework Issues that fall under the Trilinos Framework Product Area TriBITS Issues with the TriBITS framework itself, not usage of the TriBITS framework type: enhancement Issue is an enhancement, not a bug
Projects
Development

No branches or pull requests

5 participants