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

Move to modern CMake breaking Trilinos backward compatibility by changing include dirs from -I to -isystem #10160

Closed
bartlettroscoe opened this issue Feb 3, 2022 · 6 comments
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

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Feb 3, 2022

CC: @ccober6, @jwillenbring

It has been discovered that moving to modern CMake targets where downstream CMake projects pull in Trilinos include directories through IMPORTED targets results in changing the listing of the Trilinos include directories in downstream object file compiles from -I to -isystem which has several significant consequences and can break builds on fragile environments. This change is known to break at least one customer build (SPARC on the Vangard system) on where the order the include directories are searched is critical. There is an easy (but non-ideal) workaround for that particular SPARC build but it turns out that there is no way to restore backward compatibility of Trilinos include dirs without other negative consequences with CMake versions prior to 3.23 (which will not even be released until end of March 2022 or so).

The situation and the various options for working around the problem (of which are are few) are described in great detail in TriBITSPub/TriBITS#443.

After a lot of back and forth discussion with various people and Kitware, the current plan is to just accept this break in backward compatibility (since it is the CMake way) and tell users how they can workaround problems as outlined in TriBITSPub/TriBITS#443 (comment). (And once CMake 3.23 is released and people use it, then they can restore of the existing -I and -isystem includes as they are now.)

The other option that I was leaning towards before today was to have TriBITS/Trilinos place the statement:

set(CMAKE_NO_SYSTEM_FROM_IMPORTED ON CACHE BOOL "")

in all of the installed Trilinos package config files which will restore pulling in the Trilinos include dirs with -I but will also change all of the other IMPORTED target include dirs from -isystem to -I would could break customer builds and be quite undesirable.

What I need is some feedback from Trilinos developers and more Trilinos customers to decide if this is the correct course of action. We need to make a pretty firm decision on this before I can merge an updated version of PR #9978 to the 'develop' branch. (And can therefore keep making progress on the CMake modernization effort. This and relate issues has been holding up the merge of PR #9978 for over 2 months.)

@bartlettroscoe bartlettroscoe created this issue from a note in Trilinos TriBITS Refactor (Selected) Feb 3, 2022
@bartlettroscoe bartlettroscoe added PA: Framework Issues that fall under the Trilinos Framework Product Area TriBITS Issues with the TriBITS framework itself, not usage of the TriBITS framework labels Feb 3, 2022
@bartlettroscoe
Copy link
Member Author

FYI: I raised this issue briefly at the SEMS Review meeting that had several stakeholders that should know about this. (See internal meeting notes page). I will try to raise this issue as well at the next Trilinos Developers meeting on Feb 22, 2022. I am hoping before then I can get PR #9978 merged so we can be getting some more feedback to see what other customers this might be impacting.

@bartlettroscoe bartlettroscoe moved this from Selected to In Progress in Trilinos TriBITS Refactor Mar 22, 2022
@bartlettroscoe
Copy link
Member Author

Given the delay in the creation of the Trilinos 13.4 tag (see #10191), I have requested to @ccober6 that we merge the PR #9978 to Trilinos 'develop' before the 13.4 branch is created. The breaks in backward compatibility in PR #9978 are fairly rare and there are easy workarounds in most cases.

However, we should wait for CMake 3.23.0 to be released (which is currently in release candidate 4) before we pull the trigger on that merge since the only way to maintain about perfect backward compatibility in those corner cases is using CMake 3.23 and setting:

   -D Trilinos_IMPORTED_NO_SYSTEM=ON

CC: @mperrinel, @MikolajZuzek, @marcinwrobel1986, @keitat

@bartlettroscoe
Copy link
Member Author

However, we should wait for CMake 3.23.0 to be released (which is currently in release candidate 4) before we pull the trigger on that merge since the only way to maintain about perfect backward compatibility in those corner cases is using CMake 3.23 and setting:

  -D Trilinos_IMPORTED_NO_SYSTEM=ON

NOTE: CMake 3.23.0 was released on 3/29/2022 and 3.23.1 is already available (released on 4/12/2022). That means that customers now have a way to retain backward compatibility by installing and using CMake 3.23.1 and configuring Trilinos with -D Trilinos_IMPORTED_NO_SYSTEM=ON.

Note that CMake 3.23.1 can be installed using either Spack as:

$ spack external find
$ spack --no-checksum cmake@3.23~openssl

or using the simple install-cmake.py tool that is in TriBITS and Trilinos that creates a 100% independent install dir with:

$ <trilinosBaseDir>/cmake/tribits/devtools_install/install-cmake.py \
   --install-dir-base=<INSTALL_BASE_DIR> --cmake-version=3.23.1 \
   --do-all

which installs CMake executables under:

<INSTALL_BASE_DIR>/cmake-3.23.1/bin/

So if you like Spack, use Spack to install CMake 3.23.1. Or, if you are not comfortable with Spack, use install-cmake.py as per above.

So @ccober6, can I merge #9978 before the Trilinos 13.4 branch is created?

@ccober6
Copy link
Contributor

ccober6 commented Apr 14, 2022

We are hopefully that the minor release (Trilinos 13.4) can occur next week, so I would suggest waiting until then.

@bartlettroscoe
Copy link
Member Author

With there merge of PR #10614, this has been unleased on Trilinos 'develop'. For follow-up issues, see #10774.

Moving this to "In Review"

@bartlettroscoe bartlettroscoe moved this from In Progress to In Review in Trilinos TriBITS Refactor Jul 21, 2022
@bartlettroscoe
Copy link
Member Author

PR #10614 has been merged for over a month now. If there were any major lingering issues with major customers, we would have heard about them by now.

Closing as complete.

Trilinos TriBITS Refactor automation moved this from In Review to Done Aug 25, 2022
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
Projects
Development

No branches or pull requests

2 participants