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

Upgrade CMake to most current version possible for Trilinos 14.0 #10355

Closed
11 tasks done
bartlettroscoe opened this issue Mar 22, 2022 · 72 comments
Closed
11 tasks done

Upgrade CMake to most current version possible for Trilinos 14.0 #10355

bartlettroscoe opened this issue Mar 22, 2022 · 72 comments
Labels
PA: Framework Issues that fall under the Trilinos Framework Product Area type: enhancement Issue is an enhancement, not a bug

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Mar 22, 2022

CC: @trilinos/framework, @keitat, @trilinos/kokkos, @trilinos/kokkos-kernels

Child Issues:

Associated Internal Issues:

  • SEPW-520: Get CMake 3.22 installed on all Trilinos platforms
  • TRILINOSHD-184: Coordinate upgrading minimum version of CMake from 3.17 to 3.22

Description

The TriBITS refactoring work and efforts to reduce the size of TriBITS to use raw CMake would be helped if TriBITS and Trilinos could upgrade the minimum required version of CMake from 3.17 to something much newer. Ideally, we would upgrade to CMake 3.22 (released in November 18, 2021).

A sampling of the new CMake features we could exploit in TriBITS and Trilinos include:

  • Use new imported targets generated by FindHDF5.cmake not available in CMake 3.17 to simplify and robustify the Trilinos FindTPLHDF5.cmake module (see below). (CMake 3.20)
  • cmake_langauge() command: Better handle lists and function forwarding and allow for function pointers to simplify TriBITS implementation. (CMake 3.18)
  • Set ctest test Labels and Details fields at runtime (CMake 3.22). (Allows custom details about failed tests.)
  • file(CHMOD_RECURSE …) to replace Linux/bash-specific commands used to fix directory permissions on SNL systems used in Trilinos installation testing and SPARC and EMPIRE + Trilinos Integration testing.
  • Support for string(JSON) for native JSON processing to allow more robust handling of data in input files (CMake 3.19)
  • Improved HIP support may benefit Kokkos and Trilinos efforts on Frountier (CMake 3.21 for improved HIP support)
  • Expanded support for generator expressions in several CMake commands (e.g. TARGET keyword with file(GENERATE …) for CMake 3.19 and add_custom_command() and add_custom_target() for CMake 3.20)
  • CMake 3.24.0 adds the --fresh option that robustly removes the CMakeCache.txt file and the CMakeFiles/ directory. So no more having to run rm -rf CMake* before you run cmake ...!

There may be other desirable features discovered in updated CMake versions that would be of use as well.

Note that the last upgrade of the minimum CMake version for Trilinos (and TriBITS) was to 3.17 and was made back on January 28, 2021 in commit b30d808. CMake 3.17.0 was tagged on March 20, 2020 which was just 10 months before Trilinos upgraded to CMake 3.17.

Therefore, having Trilinos upgrade to CMake 3.22 in say the next 2 months (by the end of May 2022 before the Trilinos 14.0 release) would mean that Trilinos would be upgrading to CMake 2.22 more than 5 months after it was released. Given the previous upgrade to CMake 3.17 with a 8 month lag from its release, this seems not too unreasonable.

NOTE: If this drags on too long, then we should consider upgrading to CMake 3.23 since it has the added benefits:

Tasks:

  • Inquire about getting SEMS to install the module sems-cmake/3.22.z on all SEMS-supported platforms (see SEHELPD-3074) ... sems-cmake/3.22.4 and sems-cmake/3.23.1 are installed (see below)
  • Inquire about getting CMake 3.22 installed in ASC DevOps CDE modules
  • Inquire about getting CMake 3.22 installed in SPARC modules
  • Inquire about getting CMake 3.22 installed in LLNL testbeds ... cmake/3.22.4 and cmake/3.23.1 have been installed on all LLNL testbed machines (see below)
  • Inquire about getting CMake 3.22 installed on Vanguard platforms (e.g. 'stria') ... cmake/3.22.3 is installed on 'stria' (see below)
  • Verify that CMake 3.22 is installed on all critical platforms
  • Post and test MR to upgrade Trilinos PR and 'master' promotion builds to CMake 3.22+ ... See https://cee-gitlab.sandia.gov/trilinos-project/srn-ini-files/-/merge_requests/20
  • Wait for merge of https://cee-gitlab.sandia.gov/trilinos-project/srn-ini-files/-/merge_requests/20 (@e10harvey)
  • Verify that upgraded CMake not breaking any PR builds (see below)
  • Send out announcement to Trilinos stakeholders on raising minimum CMake version from 3.17 to 3.22 (see below)
  • [ ] Upgrade all ATDM Trilinos builds to CMake 3.22+
  • [ ] Verify that upgraded CMake not breaking any ATDM Trilinos builds (not worse than they already are anyway)
  • Change minimum required version of CMake in TriBITS and Trilinos to CMake 3.23
@bartlettroscoe bartlettroscoe added type: enhancement Issue is an enhancement, not a bug PA: Framework Issues that fall under the Trilinos Framework Product Area labels Mar 22, 2022
@rppawlo
Copy link
Contributor

rppawlo commented Mar 22, 2022

The last time we chose a version, we picked the highest version supported across internal sandia and external hpc machines. Looking at the sems modules, sierra modules, asc cde modules, sparc modules, and the llnl testbeds - none of them have cmake 3.22 or higher. 3.19 looks to be on most of them though. Not saying we should not do this (I would like do this), but it will be more work than last time for all app teams. Will add this to the next product leaders meeting. We can reach out to the app teams and see if they are willing to upgrade.

@bartlettroscoe
Copy link
Member Author

The last time we chose a version, we picked the highest version supported across internal sandia and external hpc machines.

@rppawlo, yes, it will be a prerequisite to get the chosen CMake version installed on all the machines first. We should start pushing that now.

We really need to get the HPC and CSE communities to get into the habit of deploying the newest CMake releases as they come out. It is trivial to install an updated CMake version on any system so we just need to get everyone in the habit of doing so.

CC: @ibaned

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Mar 28, 2022

NOTE: We have a strong motivator to get at least one of the Trilinos PR builds to use at least CMake 3.21 (see #10267). I will see if SEMS can add cmake 3.22 to the set of standard installed modules (see here).

@bartlettroscoe
Copy link
Member Author

FYI: To drive the details of getting CMake 3.22 installed everywhere, I created the internal issue SEPW-520. I will just report back here when it gets done.

NOTE: If this drags on too long, then I will upgrade this request for CMake 3.22 to CMake 3.23 since it has the added benefits:

@bartlettroscoe
Copy link
Member Author

NOTE: CMake 3.23.0 has just been released as of 6 hours ago:

But I think we need to wait at least a month before we upgrade because I am sure that there will be issues found that will be addressed in patch releases 3.23.1, 3.23.2, etc.

@bartlettroscoe

This comment was marked as outdated.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Mar 30, 2022

But since the current Spack release 0.17.1 supports CMake 3.21, that should a slam dunk for every deployment process that uses Spack.

@bartlettroscoe bartlettroscoe changed the title Upgrade to CMake 3.22 for Trilinos 14.0? Upgrade to CMake to most current version possible for Trilinos 14.0 Mar 30, 2022
@haampie
Copy link

haampie commented Mar 30, 2022

There are no backports of packages in Spack (neither trilinos nor cmake). The develop branch usually has the latest cmake a few days after release.

@bartlettroscoe
Copy link
Member Author

@haampie and @alalazo, see:

That would fix the CMake deployment problem with all future Spack releases 0.18+.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Mar 30, 2022

The develop branch usually has the latest cmake a few days after release.

@haampie, as I mentioned in:

the DevOps teams that are installing CMake on these various systems are using named releases of Spack, not the Spack develop branch.

What we need is a mechanism to allow these Spack packages like CMake and Ninja to be able to download and deploy versions of these packages that come out after the base Spack release version vX.Y.0 is tagged. I outlined a mechanism for doing that.

@haampie

This comment was marked as off-topic.

@bartlettroscoe

This comment was marked as off-topic.

@bartlettroscoe bartlettroscoe changed the title Upgrade to CMake to most current version possible for Trilinos 14.0 Upgrade CMake to most current version possible for Trilinos 14.0 Mar 30, 2022
@bartlettroscoe
Copy link
Member Author

I talked with an internal DevOps team yesterday and they mentioned one path forward is to copy the Spack package directory for CMake spack/var/spack/repos/builtin/packages/cmake/ file (along with its package.py file), put it into a custom Spack "Package Repository", and then make the changes that we need in that copy. That would override the default spack/var/spack/repos/builtin/packages/cmake/ directory and use that for CMake installs.

That is not a great long-term solution but it is something that could allow DevOps teams to install CMake 3.22+ on various platforms using the current Spack v0.17.1 (and other v0.17.z patch releases).

@haampie
Copy link

haampie commented Mar 31, 2022

They can also do spack install [email protected] on older spack versions

@bartlettroscoe
Copy link
Member Author

They can also do spack install [email protected] on older spack versions

@haampie, I did not realize that was supported. I will give that a try. If this works, that solves the problem.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Mar 31, 2022

FYI: I tested out running spack install --no-checksum [email protected] with Spack v0.17.1 (CMake 3.23.0 just got released 2 days ago) and it worked (details below).

So this would seem to be a viable solution for teams using Spack 0.17.1 to install newer versions of CMake.

The only issue that I see is that you have to use the spack install --no-checksum argument to avoid the y/N challenge (passing in -y does not work). Therefore, to use this safely, I think you need to install your main packages using checksumed binaries first using:

$ spack install <full-speck>

and then follow that up with individual calls to:

spack install --no-checksum [email protected]
Details of running spack install [email protected] using Spack 0.17.1 (click to expand)

On my SNL RHEL7 machine 'crf450' (which is not a CEE RHEL7 machine), I ran:

$ spack external find
==> The following specs have been detected on this system and added to /ascldap/users/rabartl/.spack/packages.yaml
[email protected]    [email protected]   [email protected]     [email protected]       [email protected]   [email protected]  [email protected]             [email protected]        [email protected]    [email protected]
[email protected]  [email protected]   [email protected]     [email protected]        [email protected]    [email protected]  [email protected]_272-b10  [email protected]  [email protected]     [email protected]
[email protected]      [email protected]  [email protected]         [email protected]         [email protected]    [email protected]      [email protected]          [email protected]       [email protected]      texlive@20130530
[email protected]      [email protected]     [email protected]  [email protected]  [email protected]  [email protected]   [email protected]    [email protected]       [email protected]  [email protected]

$ spack install -y [email protected]~openssl
==> Bootstrapping clingo from pre-built binaries
==> Warning: Missing a source id for [email protected]
==> Installing pkgconf-1.8.0-2zg3j5kkgcfescjiyrigrdhga7zks4bw
==> No binary for pkgconf-1.8.0-2zg3j5kkgcfescjiyrigrdhga7zks4bw found: installing from source
==> Fetching https://mirror.spack.io/_source-cache/archive/ef/ef9c7e61822b7cb8356e6e9e1dca58d9556f3200d78acab35e4347e9d4c2bbaf.tar.xz
==> No patches needed for pkgconf
==> pkgconf: Executing phase: 'autoreconf'
==> pkgconf: Executing phase: 'configure'
==> pkgconf: Executing phase: 'build'
==> pkgconf: Executing phase: 'install'
==> pkgconf: Successfully installed pkgconf-1.8.0-2zg3j5kkgcfescjiyrigrdhga7zks4bw
  Fetch: 0.21s.  Build: 7.08s.  Total: 7.29s.
[+] /home/rabartl/Spack.base/spack/opt/spack/linux-rhel7-haswell/gcc-4.8.5/pkgconf-1.8.0-2zg3j5kkgcfescjiyrigrdhga7zks4bw
[+] /usr (external openssl-1.0.2k-fips-mjjwdaachzev6hwh27yzbfb4lczuxxqk)
==> Installing ncurses-6.2-c5fr4gw5k7a3jcbkmpknzd4npswnb6pz
==> No binary for ncurses-6.2-c5fr4gw5k7a3jcbkmpknzd4npswnb6pz found: installing from source
==> Fetching https://mirror.spack.io/_source-cache/archive/30/30306e0c76e0f9f1f0de987cf1c82a5c21e1ce6568b9227f7da5b71cbea86c9d.tar.gz
==> No patches needed for ncurses
==> ncurses: Executing phase: 'autoreconf'
==> ncurses: Executing phase: 'configure'
==> ncurses: Executing phase: 'build'
==> ncurses: Executing phase: 'install'
==> ncurses: Successfully installed ncurses-6.2-c5fr4gw5k7a3jcbkmpknzd4npswnb6pz
  Fetch: 0.44s.  Build: 1m 3.56s.  Total: 1m 4.01s.
[+] /home/rabartl/Spack.base/spack/opt/spack/linux-rhel7-haswell/gcc-4.8.5/ncurses-6.2-c5fr4gw5k7a3jcbkmpknzd4npswnb6pz
==> Installing cmake-3.23.0-se23sydc6krjpdoxka2l76n3nmxdr7et
==> No binary for cmake-3.23.0-se23sydc6krjpdoxka2l76n3nmxdr7et found: installing from source
==> Fetching https://github.com/Kitware/CMake/releases/download/v3.23.0/cmake-3.23.0.tar.gz
==> No patches needed for cmake
==> cmake: Executing phase: 'bootstrap'
==> cmake: Executing phase: 'build'
==> cmake: Executing phase: 'install'
==> Warning: Module file /home/rabartl/Spack.base/spack/share/spack/modules/linux-rhel7-haswell/cmake-3.23.0-gcc-4.8.5-se23syd exists and will not be overwritten
==> cmake: Successfully installed cmake-3.23.0-se23sydc6krjpdoxka2l76n3nmxdr7et
  Fetch: 3.86s.  Build: 2m 34.36s.  Total: 2m 38.22s.
[+] /home/rabartl/Spack.base/spack/opt/spack/linux-rhel7-haswell/gcc-4.8.5/cmake-3.23.0-se23sydc6krjpdoxka2l76n3nmxdr7et

And I tested this CMake 3.23.0 install out with TriBITS and it seems to work.

@bartlettroscoe
Copy link
Member Author

NOTE: An install of CMake is less than 70M using spack install --no-checksum [email protected]. Therefore, having teams install and keep many different installs of CMake on a machine does not take up a lot of disk space. Even 20 installs of CMake at 70M is only 1.4G of space on the machine.

@bartlettroscoe
Copy link
Member Author

I wanted to post some thoughts that I had after spending many hours trying to get CMake 3.22 installed on all of these systems and communicating with lots of people (see the subtasks of the internal issue SEPW-520).


We need to have this conversation about CMake at a higher level because the only way that we can maximally leverage forward momentum in CMake feature development and bugfixes (which are substantial as new systems and compiler versions are constantly coming out) is to get all of the facilities and other systems supporting HPC and CSE customers to adjust their deployment process to make installs of new versions of CMake cheap, easy, and regular. CMake is unlike any other piece of software that I can think of in this respect. (And forcing packages to use older versions of CMake creates a lot of added waste needing to work-around issues that are resolved in newer versions of CMake.) Streamlining the CMake deployment process will completely eliminate individual requests for installing updated versions of CMake and allow projects to use whatever newer version of CMake they want/need.

The other option is to move way from the model of relying on facilities to install CMake on the system globally and instead just make it super easy for individual projects and users to install their own CMake instances on any system. Spack seems to be overkill with too many side-effects and complexities to impose on most users who just want CMake and Ninja (which can be built with just a C and C++ compiler and no other upstream dependencies).

NOTE: My current impression is that the latest version of Spack is very attractive for teams that maintain the installs and upgrades of a large number of dependent packages on a given system. In these cases, the time and investment to learn to use Spack is well worth the effort. But that is not the use case I am referring to in the above paragraph. I am just talking about installing latest versions of small executable tools packages with essentially no dependencies like CMake and Ninja (and here the real need is for updated versions of CMake, not Ninja so much).

@alalazo
Copy link

alalazo commented Apr 1, 2022

Spack seems to be overkill with too many side-effects and complexities to impose on most users who just want CMake and Ninja

In some user directory, use this spack.yaml:

spack:
  specs:
  - cmake
  - ninja
  concretization: together
  config:
    concretizer: clingo
    install_tree:
      root: ./store
  view: ./view

then:

$ ls
spack.yaml
$ spack -e . concretize -f
[ ... ]
$ spack -e . install
[ ... ]

At the end cmake and ninja should be usable by adding ${PWD}/view/bin to the PATH. To update the software, after an update of Spack:

$ spack -e . concretize -f
$ spack -e . install

bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
trilinos#10355, trilinos#11583)

The non-namespaced macro define var names `ZCPLX`, `SCPLX`, `DREAL`, and
`SREAL` are set through tribits_add_option_and_define(<optionName>
<defineName> ...) in both pliris/CMakeLists.txt and adelus/CMakeLists.txt.
This commit passed in the new option `NONCACHE` to
`tribits_add_option_and_define()` to make these macro define vars local
instead of global cache vars.  That avoids these vars being visible to other
packages and resetting the vars of the same name in the other package.  But as
local vars defined at the base package-level directory scope, the can be
accessed by any CMake code executed in the package like when the package's
`<Package>_config.h` file gets configured.

In general, one should never have non-namespaced project-level varaibles with
short names like this.  That is just asking for trouble and confusion.

Also, I removed the call to SET_CACHE_ON_OFF_EMPTY() for the initial defintion
of Adelus_ENABLE_ZCPLX.  It makes no sense for that variable's value for be
possibily empty.  All we want is a default value based on the the other set
options.  So I just used a simple set(${PACKAGE_NAME}_ENABLE_ZCPLX_DEFAULT ON)
statement to accomplish that.

SQUASH AGAINST: Adelus: Set non-namespaced macro define vars as local not global/cache (trilinos#10355, trilinos#11583)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
…S automatically sets (trilinos#10355, trilinos#11583)

The updated funtion tribits_add_option_and_define(<optionName> <defineName>
...) will error out if a local non-cache var of the same name <defineName>
already exists.  As explained in the TriBITS Users Guide, for all dependencies
listed in the package's Dependencies.cmake file, TriBITS automatically defines
and sets the vars:

* <Package>_ENABLE_<UpstreamDepPkg>: Cache var and/or non-cache local project-level var

* HAVE_<PACKAGE>_<UPSTREAMDEPPKG>: Non-cache local project-level var

The function tribits_add_option_and_define(<optionName> <defineName> ...)
creates an INTERNAL cache var with the name <defineName>.  (This was done
because some Trilinos packages communicated info through these <defineName>
vars for some reason.)

With new behavior in CMake 3.21+ with policy CMP0126, setting the INTERNAL
cache var <defineName> no longer resets the local var of the same name.
Therefore, the function tribits_add_option_and_define(<optionName>
<defineName> ...) has no impact and does not change anything that impacts
behavior.  Also, since TriBITS automatically defines documented cache vars
with the name <Package>_ENABLE_<UpstreamDepPkg>, the statement
set(<optionName> <value> CACHE BOOL ...) does not have any effect because the
cache var is already set.  Therefore, these calls to:

tribits_add_option_and_define(<Package>_ENABLE_<UpstreamDepPkg>
  HAVE_<PACKAGE>_<UPSTREAMDEPPKG> ...)

have no impact and just confused behavior.  (But were actually masking a nasty
defect described in trilinos#11583 after updating the CMake minimum version to 3.23.)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
…BITS automatically sets (trilinos#10355, trilinos#11583)

The updated funtion tribits_add_option_and_define(<optionName> <defineName>
...) will error out if a local non-cache var of the same name <defineName>
already exists.  As explained in the TriBITS Users Guide, for all dependencies
listed in the package's Dependencies.cmake file, TriBITS automatically defines
and sets the vars:

* <Package>_ENABLE_<UpstreamDepPkg>: Cache var and/or non-cache local project-level var

* HAVE_<PACKAGE>_<UPSTREAMDEPPKG>: Non-cache local project-level var

The function tribits_add_option_and_define(<optionName> <defineName> ...)
creates an INTERNAL cache var with the name <defineName>.  (This was done
because some Trilinos packages communicated info through these <defineName>
vars for some reason.)

With new behavior in CMake 3.21+ with policy CMP0126, setting the INTERNAL
cache var <defineName> no longer resets the local var of the same name.
Therefore, the function tribits_add_option_and_define(<optionName>
<defineName> ...) has no impact and does not change anything that impacts
behavior.  Also, since TriBITS automatically defines documented cache vars
with the name <Package>_ENABLE_<UpstreamDepPkg>, the statement
set(<optionName> <value> CACHE BOOL ...) does not have any effect because the
cache var is already set.  Therefore, these calls to:

tribits_add_option_and_define(<Package>_ENABLE_<UpstreamDepPkg>
  HAVE_<PACKAGE>_<UPSTREAMDEPPKG> ...)

have no impact and just confused behavior.  (But were actually masking a nasty
defect described in trilinos#11583 after updating the CMake minimum version to 3.23.)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
…utomatically sets (trilinos#10355, trilinos#11583)

The updated funtion tribits_add_option_and_define(<optionName> <defineName>
...) will error out if a local non-cache var of the same name <defineName>
already exists.  As explained in the TriBITS Users Guide, for all dependencies
listed in the package's Dependencies.cmake file, TriBITS automatically defines
and sets the vars:

* <Package>_ENABLE_<UpstreamDepPkg>: Cache var and/or non-cache local project-level var

* HAVE_<PACKAGE>_<UPSTREAMDEPPKG>: Non-cache local project-level var

The function tribits_add_option_and_define(<optionName> <defineName> ...)
creates an INTERNAL cache var with the name <defineName>.  (This was done
because some Trilinos packages communicated info through these <defineName>
vars for some reason.)

With new behavior in CMake 3.21+ with policy CMP0126, setting the INTERNAL
cache var <defineName> no longer resets the local var of the same name.
Therefore, the function tribits_add_option_and_define(<optionName>
<defineName> ...) has no impact and does not change anything that impacts
behavior.  Also, since TriBITS automatically defines documented cache vars
with the name <Package>_ENABLE_<UpstreamDepPkg>, the statement
set(<optionName> <value> CACHE BOOL ...) does not have any effect because the
cache var is already set.  Therefore, these calls to:

tribits_add_option_and_define(<Package>_ENABLE_<UpstreamDepPkg>
  HAVE_<PACKAGE>_<UPSTREAMDEPPKG> ...)

have no impact and just confused behavior.  (But were actually masking a nasty
defect described in trilinos#11583 after updating the CMake minimum version to 3.23.)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
…TS automatically sets (trilinos#10355, trilinos#11583)

The updated funtion tribits_add_option_and_define(<optionName> <defineName>
...) will error out if a local non-cache var of the same name <defineName>
already exists.  As explained in the TriBITS Users Guide, for all dependencies
listed in the package's Dependencies.cmake file, TriBITS automatically defines
and sets the vars:

* <Package>_ENABLE_<UpstreamDepPkg>: Cache var and/or non-cache local project-level var

* HAVE_<PACKAGE>_<UPSTREAMDEPPKG>: Non-cache local project-level var

The function tribits_add_option_and_define(<optionName> <defineName> ...)
creates an INTERNAL cache var with the name <defineName>.  (This was done
because some Trilinos packages communicated info through these <defineName>
vars for some reason.)

With new behavior in CMake 3.21+ with policy CMP0126, setting the INTERNAL
cache var <defineName> no longer resets the local var of the same name.
Therefore, the function tribits_add_option_and_define(<optionName>
<defineName> ...) has no impact and does not change anything that impacts
behavior.  Also, since TriBITS automatically defines documented cache vars
with the name <Package>_ENABLE_<UpstreamDepPkg>, the statement
set(<optionName> <value> CACHE BOOL ...) does not have any effect because the
cache var is already set.  Therefore, these calls to:

tribits_add_option_and_define(<Package>_ENABLE_<UpstreamDepPkg>
  HAVE_<PACKAGE>_<UPSTREAMDEPPKG> ...)

have no impact and just confused behavior.  (But were actually masking a nasty
defect described in trilinos#11583 after updating the CMake minimum version to 3.23.)

This commit also adds a formal optional dependency on MPI to replace the
option Ifpack2_ENABLE_MPI and the macro define var HAVE_IFPACK2_MPI that are
defined automatically by TriBITS.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
, trilinos#11583)

There was a defect where a call to

  tribits_add_option_and_define(ML_ENABLE_SuperLU1_0 HAVE_ML_SUPERLU ...)

was interacting poorly with the TriBITS-defined project-level local var
HAVE_ML_SUPERLU (which is created automatically by TriBITS due to the listing
of SuperLU in the ml/cmake/Dependencies.cmake file) and was discovered with
trying to upgrade the minimum CMake version from 3.17 to 3.23 (see trilinos#10355 and
3.21+, this call to tribits_add_option_and_define(ML_ENABLE_SuperLU1_0
HAVE_ML_SUPERLU ...) had the effect of setting the cache var HAVE_ML_SUPERLU
to OFF (because no one was ever setting ML_ENABLE_SuperLU1_0=ON) which was
overwritting the TriBITS-set local var HAVE_ML_SUPERLU from ON to OFF (even
when TPL_ENABLE_SuperLU=ON and ML_ENABLE_SuperLU=ON set).  So it is likely
that SuperLU support in ML was **never** enabled in any automated Trilinos
builds since Trilinos was transitioned to CMake back in 2008!

But with the upgrade of the minimum version of CMake 3.17 to 3.23 and the
CMake 3.21+ policy change, this call to

  tribits_add_option_and_define(ML_ENABLE_SuperLU1_0 HAVE_ML_SUPERLU ...)

was no longer resetting the project-level local var HAVE_ML_SUPERLU from ON to
OFF and the project-level var HAVE_ML_SPERLU was hiding the new cache var
which resulted in `#define HAVE_ML_SUPERLU` being set in the configured
ML_config.h file which attemped to enable support for SuperLU in ML for likely
the first time in 14+ years.  This resulted in compile errors because the
header files for really old versions of SuperLU like "dsp_defs.h" and "util.h"
don't exist in newer versions of SuperLU.

To fix this, the macro name HAVE_ML_SUPERLU was change to HAVE_ML_SUPERLU1_0
in the call to

  tribits_add_option_and_define(ML_ENABLE_SuperLU1_0 HAVE_ML_SUPERLU1_0 ...)

and in the file ml_config.h.in and ml_common.h.

Now, SuperLU support will not get enabled unless the users sets one of the:

  ML_ENABLE_SuperLU<X>_0

cache vars.

After this commit, someone can upgrade ML to use modern versions of SuperLU if
desired (but the approach for detecting the version of SuperLU should be
automated if possible).
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
…iBITS automatically sets (trilinos#10355, trilinos#11583)

The updated funtion tribits_add_option_and_define(<optionName> <defineName>
...) will error out if a local non-cache var of the same name <defineName>
already exists.  As explained in the TriBITS Users Guide, for all dependencies
listed in the package's Dependencies.cmake file, TriBITS automatically defines
and sets the vars:

* <Package>_ENABLE_<UpstreamDepPkg>: Cache var and/or non-cache local project-level var

* HAVE_<PACKAGE>_<UPSTREAMDEPPKG>: Non-cache local project-level var

The function tribits_add_option_and_define(<optionName> <defineName> ...)
creates an INTERNAL cache var with the name <defineName>.  (This was done
because some Trilinos packages communicated info through these <defineName>
vars for some reason.)

With new behavior in CMake 3.21+ with policy CMP0126, setting the INTERNAL
cache var <defineName> no longer resets the local var of the same name.
Therefore, the function tribits_add_option_and_define(<optionName>
<defineName> ...) has no impact and does not change anything that impacts
behavior.  Also, since TriBITS automatically defines documented cache vars
with the name <Package>_ENABLE_<UpstreamDepPkg>, the statement
set(<optionName> <value> CACHE BOOL ...) does not have any effect because the
cache var is already set.  Therefore, these calls to:

tribits_add_option_and_define(<Package>_ENABLE_<UpstreamDepPkg>
  HAVE_<PACKAGE>_<UPSTREAMDEPPKG> ...)

have no impact and just confused behavior.  (But were actually masking a nasty
defect described in trilinos#11583 after updating the CMake minimum version to 3.23.)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 23, 2023
…iBITS automatically sets (trilinos#10355, trilinos#11583)

The updated funtion tribits_add_option_and_define(<optionName> <defineName>
...) will error out if a local non-cache var of the same name <defineName>
already exists.  As explained in the TriBITS Users Guide, for all dependencies
listed in the package's Dependencies.cmake file, TriBITS automatically defines
and sets the vars:

* <Package>_ENABLE_<UpstreamDepPkg>: Cache var and/or non-cache local project-level var

* HAVE_<PACKAGE>_<UPSTREAMDEPPKG>: Non-cache local project-level var

The function tribits_add_option_and_define(<optionName> <defineName> ...)
creates an INTERNAL cache var with the name <defineName>.  (This was done
because some Trilinos packages communicated info through these <defineName>
vars for some reason.)

With new behavior in CMake 3.21+ with policy CMP0126, setting the INTERNAL
cache var <defineName> no longer resets the local var of the same name.
Therefore, the function tribits_add_option_and_define(<optionName>
<defineName> ...) has no impact and does not change anything that impacts
behavior.  Also, since TriBITS automatically defines documented cache vars
with the name <Package>_ENABLE_<UpstreamDepPkg>, the statement
set(<optionName> <value> CACHE BOOL ...) does not have any effect because the
cache var is already set.  Therefore, these calls to:

tribits_add_option_and_define(<Package>_ENABLE_<UpstreamDepPkg>
  HAVE_<PACKAGE>_<UPSTREAMDEPPKG> ...)

have no impact and just confused behavior.  (But were actually masking a nasty
defect described in trilinos#11583 after updating the CMake minimum version to 3.23.)
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 23, 2023
…defined (trilinos/Trilinos#10355)

This causes tribits_add_option_and_define(<optionName> <defineName> ...) to
error out if <optionName> or <defineName> are already defined as a non-cache
vars.  This is needed to catch tricky cases that were likely an accident where
a package's CMakeLists.txt file was trying to redefine a TriBITS-set
project-level non-cache var (e.g. HAVE_ML_SUPERLU in trilinos/Trilinos#11583)
which can casue a nasty change in behavior when using the new behvior with
CMake 3.21+ with policy CMP0126 where setting cache vars no longer hides local
vars.  (See Trilinos Issue trilinos/Trilinos#10355 and Trilinos PR
trilinos/Trilinos#11583.)

NOTE: I also changed this from a macro to a function.  There is no reason this
needed to be a macro and a function is much safer as all of the local vars
created in the funtion will not bleed into the calling scope (only the one's
we explicitly set using PARENT_SCOPE).  I also added the NOCACHE option for
cases where the <defineName> should never be made a global var and should
never be exported to other projects.  (The NOCACHE option will get used for
some non-namespaced vars from the pakages Pliris and Adelus.)
bartlettroscoe added a commit to TriBITSPub/TriBITS that referenced this issue Feb 23, 2023
…he-vars

tribits_add_option_and_define(): Error out if non-cache vars already defined (trilinos/Trilinos#10355)

See trilinos/Trilinos#10355 and trilinos/Trilinos#11583
@bartlettroscoe
Copy link
Member Author

FYI: PR #11583 was just merged. Now we see if anyone has a problem with this.

jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue Feb 24, 2023
…s:develop' (cc7c267).

* trilinos-develop:
  TpetraTsqr: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  ShyLU_Node: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  ML: Change name of HAVE_ML_SUPERLU to HAVE_ML_SUPERLU1_0 (trilinos#10355, trilinos#11583)
  Ifpack2: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  FEI: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  EpetraExt: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  Amesos: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  Adelus: Set non-namespaced macro define vars as local not global/cache (trilinos#10355, trilinos#11583)
  Pliris: Set non-namespaced macro define vars as local not global/cache (trilinos#10355, trilinos#11583)
  tribits_add_option_and_define(): Error out if non-cache vars already defined (trilinos#10355)
  Update CMake min version from 3.17 to 3.23 (trilinos#10355)
  NEMESIS: Linker language is messing up library dependency on cuda build
  NEMESIS: See if this will solve link issue
  Remove a few IOSS_MAYBE_UNUSED that seem to cause gcc-8.3 errors
  removing comments for cxx standard
  Automatic snapshot commit from seacas at d4492cbf0f
  Display CXX Standard to the output of the CMakeLists.txt file
  test/build Trilinos nightly for CXX 20
jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue Feb 24, 2023
…s:develop' (cc7c267).

* trilinos-develop:
  TpetraTsqr: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  ShyLU_Node: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  ML: Change name of HAVE_ML_SUPERLU to HAVE_ML_SUPERLU1_0 (trilinos#10355, trilinos#11583)
  Ifpack2: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  FEI: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  EpetraExt: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  Amesos: Remove tribtits_add_option_and_define() calls for vars TriBITS automatically sets (trilinos#10355, trilinos#11583)
  Adelus: Set non-namespaced macro define vars as local not global/cache (trilinos#10355, trilinos#11583)
  Pliris: Set non-namespaced macro define vars as local not global/cache (trilinos#10355, trilinos#11583)
  tribits_add_option_and_define(): Error out if non-cache vars already defined (trilinos#10355)
  Update CMake min version from 3.17 to 3.23 (trilinos#10355)
  NEMESIS: Linker language is messing up library dependency on cuda build
  NEMESIS: See if this will solve link issue
  Remove a few IOSS_MAYBE_UNUSED that seem to cause gcc-8.3 errors
  removing comments for cxx standard
  Automatic snapshot commit from seacas at d4492cbf0f
  Display CXX Standard to the output of the CMakeLists.txt file
  test/build Trilinos nightly for CXX 20
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 25, 2023
This is part of allowing TriBITS to take advantage of newer CMake features and
replacing some older TriBITS code that now has (better or compatiable) native
implementations in CMake (many years later).

The min version of CMake 3.23 is chosen due to Trilinos (see
trilinos/Trilinos#10355).

Some non-obvious changes made were:

* For some reason, find_file() wiht CMake 3.23 (and using CMak 3.23 default
  policies) adds on an extra '/' before the file name.  This messed up the
  test checks but I fixed that by putting in '[/]*' in the regex to account
  for that.

* Updated the GitHub Actions testing jobs to all be CMake 3.23+.  (I updated
  the patch versions as well to the most recent patch releases as of right
  now.  CMake 3.25 is still the most recent release of CMake as CMake 3.26 is
  still in release candidate testing.)

* Add checks for setting CTEST_RESOURCE_SPEC_FILE in CTestTestfile.cmake now
  that CMake is >= 3.18.

* Removed constant ${PROJECT_NAME}_CTEST_USE_NEW_AAO_FEATURES since it is
  hard-coded since CMake is 3.17+.

* Removed some documentation discussing older versions of CMakew (which are no
  longer supported).

* install-cmake.py: Changed the default version of CMake to install from 3.17
  to 3.23.

* install-cmake.py: Removed patch for CMake 3.17

* Removed documentation for Kitware fork of Ninja version < 1.10 since Ninja
  1.10 has been out for many years.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 25, 2023
This is part of allowing TriBITS to take advantage of newer CMake features and
replacing some older TriBITS code that now has (better or compatiable) native
implementations in CMake (many years later).

The min version of CMake 3.23 is chosen due to Trilinos (see
trilinos/Trilinos#10355).

Some non-obvious changes made were:

* For some reason, find_file() wiht CMake 3.23 (and using CMak 3.23 default
  policies) adds on an extra '/' before the file name.  This messed up the
  test checks but I fixed that by putting in '[/]*' in the regex to account
  for that.

* Updated the GitHub Actions testing jobs to all be CMake 3.23+.  (I updated
  the patch versions as well to the most recent patch releases as of right
  now.  CMake 3.25 is still the most recent release of CMake as CMake 3.26 is
  still in release candidate testing.)

* Add checks for setting CTEST_RESOURCE_SPEC_FILE in CTestTestfile.cmake now
  that CMake is >= 3.18.

* Removed constant ${PROJECT_NAME}_CTEST_USE_NEW_AAO_FEATURES since it is
  hard-coded since CMake is 3.17+.

* Removed some documentation discussing older versions of CMakew (which are no
  longer supported).

* install-cmake.py: Changed the default version of CMake to install from 3.17
  to 3.23.

* install-cmake.py: Removed patch for CMake 3.17

* Removed documentation for Kitware fork of Ninja version < 1.10 since Ninja
  1.10 has been out for many years.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 25, 2023
This is part of allowing TriBITS to take advantage of newer CMake features and
replacing some older TriBITS code that now has (better or compatiable) native
implementations in CMake (many years later).

The min version of CMake 3.23 is chosen due to Trilinos (see
trilinos/Trilinos#10355).

Some non-obvious changes made were:

* For some reason, find_file() wiht CMake 3.23 (and using CMak 3.23 default
  policies) adds on an extra '/' before the file name.  This messed up the
  test checks but I fixed that by putting in '[/]*' in the regex to account
  for that.

* Updated the GitHub Actions testing jobs to all be CMake 3.23+.  (I updated
  the patch versions as well to the most recent patch releases as of right
  now.  CMake 3.25 is still the most recent release of CMake as CMake 3.26 is
  still in release candidate testing.)

* Add checks for setting CTEST_RESOURCE_SPEC_FILE in CTestTestfile.cmake now
  that CMake is >= 3.18.

* Removed constant ${PROJECT_NAME}_CTEST_USE_NEW_AAO_FEATURES since it is
  hard-coded since CMake is 3.17+.

* Removed some documentation discussing older versions of CMakew (which are no
  longer supported).

* install-cmake.py: Changed the default version of CMake to install from 3.17
  to 3.23.

* install-cmake.py: Removed patch for CMake 3.17

* Removed documentation for Kitware fork of Ninja version < 1.10 since Ninja
  1.10 has been out for many years.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 25, 2023
This is the first step in allowing TriBITS to take advantage of newer CMake
features and replacing some older TriBITS code that now has (better or
compatible) native implementations in CMake (many years after the TriBITS
support was created).

The min version of CMake 3.23 is chosen due to Trilinos changing its min
version to 3.23 (see trilinos/Trilinos#10355).

Some non-obvious changes made in this commit were:

* For some reason, find_file() with CMake 3.23 (and using CMake 3.23 default
  policies) adds on an extra '/' before the file name in the found path.  This
  messed up some of the test checks in TriBITS but I fixed that by putting
  '[/]*' in the regex to account for that extra '/'.

* Updated the GitHub Actions testing jobs to all be CMake 3.23+.  (I updated
  the patch versions of the different versions as well to the most recent
  patch releases for those minor versions as of right now.  CMake 3.25 is
  still the most recent release of CMake as CMake 3.26 is still in release
  candidate testing.  Therefore, CMake 3.26.2 is still the most current
  release of CMake.)

* Added checks for setting CTEST_RESOURCE_SPEC_FILE in CTestTestfile.cmake now
  that CMake is >= 3.18.

* Removed constant ${PROJECT_NAME}_CTEST_USE_NEW_AAO_FEATURES since it is
  hard-coded now that CMake is 3.17+ is required.

* Removed some documentation discussing older versions of CMake (which are no
  longer supported by TriBITS).

* install-cmake.py: Changed the default version of CMake to install from 3.17
  to 3.23.

* install-cmake.py: Removed patch for CMake 3.17

* Removed documentation for the Kitware fork of Ninja versions < 1.10 since
  Ninja 1.10 has been out for several years with Fortran support.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Feb 25, 2023
This is part of allowing TriBITS to take advantage of newer CMake features and
replacing some older TriBITS code that now has (better or compatible) native
implementations in CMake (often many years after the TriBITS support was
created).  This allows unconditionally using CMake features up to version 3.23
(where we were limited unconditionally using features up to CMake version
3.17).

The min version of CMake 3.23 is chosen due to Trilinos changing its min
version to 3.23 (see trilinos/Trilinos#10355).

Some non-obvious changes made in this commit were:

* For some reason, find_file() with CMake 3.23 (and using CMake 3.23 default
  policies) adds on an extra '/' before the file name in the found path.  This
  messed up some of the test checks in TriBITS but I fixed that by putting
  '[/]*' in the regex to account for that extra '/'.

* Updated the GitHub Actions testing jobs to all be CMake 3.23+.  (I updated
  the patch versions of the different versions as well to the most recent
  patch releases for those minor versions as of right now.  CMake 3.25 is
  still the most recent release of CMake as CMake 3.26 is still in release
  candidate testing.  Therefore, CMake 3.26.2 is still the most current
  release of CMake.)

* Added checks for setting CTEST_RESOURCE_SPEC_FILE in CTestTestfile.cmake now
  that CMake is >= 3.18.

* Removed constant ${PROJECT_NAME}_CTEST_USE_NEW_AAO_FEATURES since it is
  hard-coded now that CMake is 3.17+ is required.

* Removed some documentation discussing older versions of CMake (which are no
  longer supported by TriBITS).

* install-cmake.py: Changed the default version of CMake to install from 3.17
  to 3.23.

* install-cmake.py: Removed patch for CMake 3.17

* Removed documentation for the Kitware fork of Ninja versions < 1.10 since
  Ninja 1.10 has been out for several years with Fortran support.
bartlettroscoe added a commit to TriBITSPub/TriBITS that referenced this issue Feb 25, 2023
trilinos-autotester added a commit that referenced this issue Mar 3, 2023
Automatically Merged using Trilinos Pull Request AutoTester
PR Title: Upgrade to CMake 3.23 (#10355)
PR Author: bartlettroscoe
@bartlettroscoe
Copy link
Member Author

This was merged Feb 16 in PR:

and some documentation was fixed in PR:

That is nearly 2 months since this change and this went out in Trilinos 14.0 last month.

Therefore, I think that is enough time to leave this issue in review and it is time to close.

NOTE: there was a question if this was a "strict" upgrade 3 days ago in TriBITSPub/TriBITS#565 (comment) by a staff member at LANL (see here) . They said there were some systems they were working on that did not have CMake 3.23+. I asked what systems those were but there has not been any response yet. I fully believe that that there are systems at some institutions that don't have updated installs of CMake. But CMake 3.23 was released over a year ago (see here). That should be enough time for developers and institutions to get upgraded versions of CMake installed.

Closing as complete.

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 type: enhancement Issue is an enhancement, not a bug
Projects
Development

No branches or pull requests

10 participants