Investigate and address issue with change to -isystem includes for IMPORTED targets (#299) #443
Comments
bartlettroscoe
changed the title
|
To summarize how the SPARC build error occurred, 5 things all had to occur on this system as outlined in tribitsrefactoring#3 which are:
All 5 of these things had to happen in order for this error to occur. If you address/change any one of these 5 things (with the suggested fixes or workarounds), then the this error goes away and SPARC builds and runs fine. Basically, this is just a fragile system setup and is begging for trouble. We have created the problem for ourselves by:
|
|
FYI: I have asked Kitware for their advice on how to deal with this in: |
|
CMake 3.23 introduces the IMPORTED_NO_SYSTEM target property that can be set on an imported target to prevent its include directories from being treated as system includes. |
@bradking, excellent! That matches my suggested property So with CMake 3.22.0 being released on 2021-11-18, should we expect CMake 3.23.0 to be released at end of March 2022 (i.e. a new CMake release every 4 months)? But even if So waiting for and requiring CMake 3.23 does not seem like a reasonable uniform solution. (But it could be a good solution for some customers that have very challenging fragile environments so we should still support So without CMake 3.23, as a non-ideal workaround, what do you think of having TriBITS, by default, add: to the What do you think? I need some advice here. |
|
Just to give some more detail on what is happening with the SPARC build on 'stria' with that fragile env and the gtest problem, as described in the internal issue tribitsrefactoring#3 ... The old TriBITS for Trilinos results in the SPARC compile include dirs using Note that the above means that:
The new TriBITS without setting The big change here is that the Trilinos include dir is now listed at the end of the compile line with And finally, the new TriBITS with setting This restores the NOTE: I also experimented that setting |
|
@bradking, While we are discussing CMake features, what about allowing the downstream CMake projects more fine-grained control on how they treat include directories for upstream IMPORTED targets like suggested above? Yes, the new CMake 3.23 property |
|
@bradking, is it possible for a downstream CMake project to edit the properties on an IMPORTED target after they are defined? For example, could a downstream CMake project change the value of the I could experiment with this to see but I though it would be worth asking if this is a supported CMake behavior before I try. |
I think that would be okay so long as it is guarded by some option the project can set to avoid it.
For reference, that suggestion is:
It might be possible but will require deep design work (click to expand details):The
Yes. This might be the best approach if the motivating problem only affects a few projects. |
I think what we would do is to have a TriBITS project-level cache var option But for CMake 3.23+, we would instead have a TriBITS project-level cache var option That way, if a customer grabbed an updated version of TriBITS or Trilinos with CMake 3.23+, they would get the about exact same handling of include dirs as with the old TriBITS. But if they used an updated TriBITS or Trilinos with CMake < 3.23, then they would still be including Trilinos headers with And we would have detailed documentation for TriBITS and Trilinos (and release notes) that explained all of this and described the options What do you think about that?
Excellent! Is that documented anywhere? I am assuming you need to adjust these IMPORTED properties before using the IMPORTED target in a Also, for this to be useful, I think we need CMake 3.23+ so we can manipulate the
It might be nice to start to carefully document the current behavior of all of these target properties and how they impact how the include directories are treated by downstream targets. For example, you mix and match the properties |
|
NOTE: I just got confirmation from the SPARC developers that they want to keep pulling in the Trilinos include dirs with This is just more motivation to set |
Having those options with defaults based on the version of CMake sounds fine to me.
AFAIK there is no documentation explicitly encouraging or discouraging that. There is no mechanism to enforce it either way.
There is documentation at the bottom of this section in the |
Thanks @bradking, I think we have a workable plan above then. Thanks for your feedback and perspective.
There are several things about include directories that seems to be unspecified. For example, how are the include directories listed in an IMPORTED target's What I did not realize until reading the CMake documentation for Also, what the CMake documentation fails to mention is that marking a directory as |
I opened CMake MR 6942 to update the documentation for those.
It's not affected. I included that in the documentation update.
I've also mentioned the ordering change in the documentation update. FYI, compilers always search |
@bradking, are there compilers that don't support silencing warnings from system headers? |
|
There are compilers that only have |
|
NOTE: If any an include directory from any target is listed in the property |
|
After talking with @bradking and others at the TriBITS Refactoring meeting, I think we have come to the following decision:
|
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…Pub#443) * Mentioned dependence on CMake 3.23+ * Mentioned the behavior differences between -I and -isystem * Mentioned it restores backward compatibility for the move to modern CMake * Fixed typos and misspellings
…ty with -isystem include dirs Hopefully this will be enough for Trilinos users to work through problems with changes in the handling of include directories. For more details, see TriBITSPub/TriBITS#443
|
@mperrinel, can you please do a detailed review of the following PRs as connected set:
? Once you are finished that, can you please review (with a formal GitHub review with comments) the PRs listed under the "In Review" column in order shown in: ? |
|
@bartlettroscoe |
|
@bartlettroscoe |
There were only a few small typos. Most of the changes were what I consider improvements to make the text more clear.
Better document <Project>_IMPORTED_NO_SYSTEM and workarounds (#443)
|
@MikolajZuzek, see this issue. This is where we break backward compatibly. |
…ty with -isystem include dirs Hopefully this will be enough for Trilinos users to work through problems with changes in the handling of include directories. For more details, see TriBITSPub/TriBITS#443
CC: @mperrinel, @marcinwrobel1986, @keitat
Parent Issues:
Other Linked issues:
Description of the problem
While studying a build errors reported with SPARC on the Vanguard system
van1-tx2on the machine 'stria' using the ARM C++ 20.1 compiler (see tribitsrefactoring#3 and SPAR-1074) for the updated TriBITS implementation increment to modern CMake targets in trilinos/Trilinos#9978 (which is reapplying and fixing trilinos/Trilinos#9894 after it was reverted), I traced the problem to a change in how include directories and specified when changing to modern cmake IMPORTED targets.With old TriBITS, a downstream customer CMake project would get the Trilinos include directories by either setting the directory-level
INCLUDE_DIRECTORIESproperty with:or at the target level with:
and the project could either include these with
SYSTEMor not and have them included on the compile line with-isystemor just-I, respectively. SPARC and many other application codes usually omitSYSTEMso the Trilinos include dirs are specified as-Ion the compile lines of the downstream customer object builds.But with this change to modern CMake IMPORTED targets as per trilinos/Trilinos#9894 (and then again after the revert of that with trilinos/Trilinos#9978) to have the
INTERFACE_INCLUDE_DIRECTORIESproperty set, by default, downstream targets add those include directories asSYSTEMincludes and apply them as-isystemon the compile lines. This changes the search order for header files and also silences warnings coming from those header files as noted in "Professional CMake: 10th edition" which states:In the case of this SPARC build on 'stria', this change of the Trilinos install include dir from
-Ito-isystemresults ingtest/gtest.hbeing found in theINCLUDEenv path var under:(because yaml-cpp installs with gtest by default, see jbeder/yaml-cpp#539) instead of under the Trilinos install dir:
(because Trilinos installs gtest for some customers, see trilinos/Trilinos#8001).
Yes, this a crazy system where the compiler (arm-20.1 C++ in this case) searches for headers in directories listed in an env var
INCLUDEbefore it searches directories listed in explicit-isystemarguments but that is what it does. And yes, it is terrible that libraries like Trilinos and yaml-cpp are installing gtest, but that is what we have.This is a fragile environment where header files of the same name are found under multiple include directories and the search order of the include directories is critical. But this is often what happens on real systems and a change in how include directories are handled can be very difficult to deal with unless you are given precise control of how include directories are handled and passed to the compiler.
Possible workarounds that can be implemented in TriBITS
TriBITS Workaround
#1 All CMake versions: Set cache varCMAKE_NO_SYSTEM_FROM_IMPORTED=ONby default inTrilinosConfig.cmake:Details (click to expand)
.
One workaround that confirmed worked (see here) is to set
CMAKE_NO_SYSTEM_FROM_IMPORTED=TRUEby adding the statement:to the installed file
<Project>Config.cmake. That resulted in all of the IMPORTED targets include directories to change from-isystemto-Iand it fixed the particular SPARC build issue. But it also changed from-isystemto-Ifor the includes of some non-Trilinos external packages and therefore has the potential of exposing the project to new build warnings and changing what header files are found for these packages.Rather than hard-code setting
CMAKE_NO_SYSTEM_FROM_IMPORTED=ON, a TriBITS project could provide a configure-time option called something like:and if
TRUE, would add the statementset(CMAKE_NO_SYSTEM_FROM_IMPORTED ON CACHE BOOL "")to all of the installed<Project>Config.cmakeand<Package>Config.cmakefiles.'But this does not maintain backwards compatibility because it also changes all of the IMPORTED targets (including non-TriBITS packages) include directories from
-isystemto-Iwhich can break the build.-I-isystemto-Iwhich can break the buildTriBITS Workaround
#2: For CMake versions >= 3.23: SetIMPORTED_NO_SYSTEMon all generated IMPORTED targetsDetails (click to expand)
.
But for CMake 3.23+, we would have a TriBITS project-level cache var option
<Project>_IMPORTED_NO_SYSTEMthat would be set toTRUEby default that would set the IMPORTED target propertyIMPORTED_NO_SYSTEMon all of the generated TriBITS package targets. And with<Project>_SET_CMAKE_NO_SYSTEM_FROM_IMPORTED=FALSEby default, that would give about perfect backwards compatibility.-Iwithout changing include dirs for other IMPORTED targets (i.e. maintains backward compatibiilty)Possible workarounds that can be implemented in the customer CMake projects
Customer Workaround
#1: All CMake versions: Customer CMake projects setCMAKE_NO_SYSTEM_FROM_IMPORTED=TRUE.Details (click to expand>
.
The downstream customer CMake project can set the cache var
CMAKE_NO_SYSTEM_FROM_IMPORTED=TRUEor set the target property NO_SYSTEM_FROM_IMPORTED=TRUE at a more fine-grained level. But this will also have the blunt-force effect of changing all of the IMPORTED targets include dirs from-isystemto-Iwhich breaks backwards compatibility for how Trilinos used to work with old TriBITS.The downstream customer could also address the header search problem directly be removing undesirable search directories, undesirable header files, etc. But that is not always possible (at least not without help from the sysadmins and other people maintaining these systems and software stacks).
But even if the header file search issue is addressed, changing from
-Ito-isystemfor included TriBITS packages will silence warnings that may otherwise have been desirable for the consuming project to see. For example, the Kokkos header files will generate useful warnings if Kokkos is used incorrectly and we don't want to silence these warnings by including the Kokkos headers with-isystem.-I-isystemto-Iwhich can break the buildCustomer Workaround
#2: All CMake versions: Directly address the duplicate header file finds:Examples:
Pros and cons:
Desired features in CMake to address the problem
The best approach would be for CMake to provide more control for how include directories are handled on a per-target basis both from the IMPORTED target itself and for consumers of IMPORTED targets to have fine-grained control for how include directories for specific upstream targets are handled.
Details (click to expand)
.
@bradking noted below that CMake 3.23 will introduce the IMPORTED_NO_SYSTEM target property that, if set to
TRUE, then theINTERFACE_INCLUDE_DIRECTORIESproperty from that IMPORTED target would be specified as-Ibut the other IMPORTED targets include directories would stay as-isystem. That would allow TriBITS to set the propertyIMPORTED_NO_SYSTEM=TRUEby default on all generated targets and therefore maintain perfect backwards compatibility for downstream customers CMake projects. (But this should be a TriBITS configure-time option to set that or not.)Also, it would be good if CMake would allow downstream CMake projects to set the
SYSTEMor no-SYSTEMhandling for linked-in IMPORTED targets include directories on an upstream target-by-target basis like:And with that, the
INTERFACE_INCLUDE_DIRECTORIESfrom<target1>and<target2>would be listed on compile line with-Iand those with<target3>and<target4>would be listed on compile line with-isystem.That would return full control of how header files are handled in the downstream CMake project that is lost with the move to modern CMake.
NOTE: Until CMake supports this (or if it never supports this), one can change the
IMPORTED_NO_SYSTEMproperty on an IMPORTED library target after it has been formed in the downstream CMake project in thefind_package()but before it gets used in atarget_link_libraries()command. The same is true for theIMPORTED_SYSTEM_INCLUDE_DIRECTORIESlist (but in that case, one must be careful to keep that in sync with theIMPORTED_INCLUDE_DIRECTORIESproperty). With those manual IMPORTED target property tweaks, the downstream CMake project should be able to set or unset theSYSTEMproperties of any of the include directories of upstream IMPORTED libraries used in the project. (We would need to do some detailed experiments to see if this is correct and what the limits to this may be. See below)Tasks
CMAKE_NO_SYSTEM_FROM_IMPORTED=ONfor the SPARC build on 'stria' as a possible workaround for them ... SPARC ended up rejecting that and going with their own version of gtest<Project>_IMPORTED_NO_SYSTEMoption (defaultFALSE) to setIMPORTED_NO_SYSTEMon all generated IMPORTED targets with CMake 3.23+. (@bartlettroscoe) ... See TriBITS PR Add support for <Project>_IMPORTED_NO_SYSTEM and related (#443) #456-Iwithinclude_directories(${<Project>_INCLUDE_DIRS)to-isystemwith modern CMake IMPORTED targets pulled in from installed package config cmake files. (@bartlettroscoe) ... See TriBITS PR Add support for <Project>_IMPORTED_NO_SYSTEM and related (#443) #456[ ] Add section in the TriBITS users and maintainers guides on the handing of include directories and how to control them and resolve issues in customer CMake projects.Not necessary given existing documentation and updated Trilinos/RELEASE_NOTES.The text was updated successfully, but these errors were encountered: