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

Add version check and calls to cmake_minimum_required() (#522) #523

Merged

Conversation

bartlettroscoe
Copy link
Member

Addresses #522. See git commit log msg for more details.

This addresses an issue where Kokkos updates the KokkosConfig_install.cmake
file to use 'if (... IN_LIST ...)'.

NOTE: There is no test in TriBITS that checks for this change because the
TriBITS-generated <Package>Config.cmake and <Project>Config.cmake files don't
use 'if (... IN_LIST ...)'.  I had to manually test this against Trilinos to
verify this makes the error go away.  All that the TriBITS testing does is
ensure that this change does not break these files for the TriBITS use cases.
@bartlettroscoe
Copy link
Member Author

@KyleFromKitware, can you please give this a quick review.

I might merge and allow you to do a post-merge review depending on how my Trilinos testing goes. I want to get a Trilinos PR with an updated TriBITS 'master' posted as soon as I can.

@bartlettroscoe
Copy link
Member Author

I tested this against Trilinos branch and version:

$ gitdist-status --dist-repos=.,TriBITS
---------------------------------------------------------------------------------------------------------
| ID | Repo Dir        | Branch                     | Tracking Branch                       | C | M | ? |
|----|-----------------|----------------------------|---------------------------------------|---|---|---|
|  0 | Trilinos (Base) | 10774-final-fixes          | rab-github/10774-final-fixes          |   |   |   |
|  1 | TriBITS         | 522-cmake-minimum-required | rab-github/522-cmake-minimum-required |   |   |   |
---------------------------------------------------------------------------------------------------------

$ gitdist-repo-versions --dist-repos=.,TriBITS
*** Base Git Repo: Trilinos
d03c5842af4a9d6bce3ef86ef10f2a596ef220cf [Wed Aug 24 16:30:16 2022 -0600] <rabartl@sandia.gov>
Change cmake_minimum_required() from 3.17.1 to 3.0 (TriBITSPUb/TriBITS#522)
*** Git Repo: TriBITS
ab41942990e8d3655463551036a979c674b27fcc [Wed Aug 24 16:50:38 2022 -0600] <rabartl@sandia.gov>
Add version check and calls to cmake_minimum_required() (#522)

on 'crf450' with:

$ ssh crf450

$ cd ~/Trilinos.base/BUILDS/PR/clang-10.0.0/

$ cat load-env.sh
export TRILINOS_DIR=${HOME}/Trilinos.base/Trilinos
source ${TRILINOS_DIR}/packages/framework/GenConfig/gen-config.sh \
--cmake-fragment GenConfigSettings.cmake \
rhel7_sems-clang-10.0.0-openmpi-1.10.1-serial_release-debug_shared_no-kokkos-arch_no-asan_no-complex_no-fpic_mpi_no-pt_no-rdc_no-uvm_deprecated-on_no-package-enables \
--force -y \
"$@" \
${TRILINOS_DIR}

$ cat do-configure
rm -rf CMakeCache.txt CMakeFiles
cmake \
-G Ninja \
-C GenConfigSettings.cmake \
-D Trilinos_TRIBITS_DIR:STRING=TriBITS/tribits \
-D Trilinos_ENABLE_ALL_FORWARD_DEP_PACKAGES=OFF \
-D Trilinos_ENABLE_TESTS=ON \
"$@" \
${TRILINOS_DIR}

$ time ./do-configure -DTrilinos_ENABLE_ALL_PACKAGES=ON &> configure.out

real    1m48.887s
user    1m5.629s
sys     0m44.551s

$ time make &> make.out 

real    86m22.995s
user    2521m31.180s
sys     62m0.112s

$ time ctest -j12 &> ctest.out

real    14m46.608s
user    149m46.226s
sys     13m18.514s

$ grep "failed out of" ctest.out 
100% tests passed, 0 tests failed out of 3398

Copy link
Collaborator

@rabartlett1972 rabartlett1972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will go ahead and allow the merge so that I can get a Trilinos PR put up so it can be tested overnight.

@bartlettroscoe bartlettroscoe merged commit 8817a8f into TriBITSPub:master Aug 25, 2022
@bartlettroscoe
Copy link
Member Author

@KyleFromKitware, can you please do a post-merge review of this PR?

bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Aug 25, 2022
…REQUIRED (TriBITSPub#522)

This was changed from the open-ended ${CMAKE_VERSION}.  This way, we allow
CMake to use a more current set of policies while not writting future futures
of CMake a blank check.

This change was made based on feedback from @KyleFromKitware from the
post-merge review of TriBITSPub#523.

NOTE: I manaully tested this with a configure of Trilinos and it produded:

  cmake_minimum_required(VERSION 3.3...3.17.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants