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

added terminal output showing compile options selected #3291

Conversation

shimwell
Copy link
Member

@shimwell shimwell commented Feb 4, 2025

Description

This PR adds a small amount of information to the user so that they can see which options were used to compile openmc

this is useful for CI when building different options

currently cmake .. outputs

-- Configuring DAGMC 3.2.4
-- Found DAGMC: /home/jon/code_development/DAGMC/lib/cmake/dagmc (version 3.2.4)
-- HDF5 C compiler wrapper is unable to compile a minimal HDF5 program.
-- HDF5 Libraries: /usr/lib/x86_64-linux-gnu/hdf5/serial/libhdf5.so
-- OpenMC C++ flags: 
-- OpenMC Linker flags: 
-- Submodule update
-- Found pugixml: /usr/local/lib/cmake/pugixml (version 1.10)
-- Found fmt: /usr/local/lib/cmake/fmt (version 11.0.2)
-- Found xtensor: /usr/local/share/cmake/xtensor (version 0.25.0)
-- Found gsl-lite: /usr/local/lib/cmake/gsl-lite (version 0.36.1)
-- Did not find Catch2, will use submodule instead
-- Configuring done (0.1s)
-- Generating done (0.0s)
-- Build files have been written to: /home/jon/openmc_fork/build

This PR would add a little info to the start and the cmake .. terminal output would look like this

-- OPENMC_USE_OPENMP ON
-- OPENMC_BUILD_TESTS ON
-- OPENMC_ENABLE_PROFILE OFF
-- OPENMC_ENABLE_COVERAGE OFF
-- OPENMC_USE_DAGMC ON
-- OPENMC_USE_LIBMESH OFF
-- OPENMC_USE_MPI OFF
-- OPENMC_USE_MCPL OFF
-- OPENMC_USE_NCRYSTAL OFF
-- OPENMC_USE_UWUW OFF
-- Configuring DAGMC 3.2.4
-- Found DAGMC: /home/jon/code_development/DAGMC/lib/cmake/dagmc (version 3.2.4)
-- HDF5 C compiler wrapper is unable to compile a minimal HDF5 program.
-- HDF5 Libraries: /usr/lib/x86_64-linux-gnu/hdf5/serial/libhdf5.so
-- OpenMC C++ flags: 
-- OpenMC Linker flags: 
-- Submodule update
-- Found pugixml: /usr/local/lib/cmake/pugixml (version 1.10)
-- Found fmt: /usr/local/lib/cmake/fmt (version 11.0.2)
-- Found xtensor: /usr/local/share/cmake/xtensor (version 0.25.0)
-- Found gsl-lite: /usr/local/lib/cmake/gsl-lite (version 0.36.1)
-- Did not find Catch2, will use submodule instead
-- Configuring done (0.1s)
-- Generating done (0.0s)
-- Build files have been written to: /home/jon/openmc_fork/build

Fixes # (issue)

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)

@gridley
Copy link
Contributor

gridley commented Feb 9, 2025

For CI, why not just check the CMakeCache file?

@shimwell
Copy link
Member Author

For CI, why not just check the CMakeCache file?

I guess that would also work in some cases, but I think having terminal output in the CI logs is a bit more convenient than the connecting to the CI runner and opening a file.

The logs are also available for longer than the ability to connect to a runner and access files which means reviewers can see from the logs if something was built correctly sometime after the PR.

@gridley
Copy link
Contributor

gridley commented Feb 10, 2025

Oh I see, you meant for debugging, not scripting within CI. I was confused. looks good to me then!

@makeclean
Copy link
Contributor

Isn't what you're looking for already there from the CI runner, i.e.
image

@gridley
Copy link
Contributor

gridley commented Feb 10, 2025

Isn't what you're looking for already there from the CI runner, i.e. image

I think the difference is that the CI permutations don't cover a few of the compile-time config flags, and more might be added in the future. Those would be:

-- OPENMC_BUILD_TESTS ON
-- OPENMC_ENABLE_PROFILE OFF
-- OPENMC_ENABLE_COVERAGE OFF
-- OPENMC_USE_UWUW OFF

@shimwell
Copy link
Member Author

Yes that name of the CI task is also super useful, I believe it is made from the CI matrix settings here

name: "Python ${{ matrix.python-version }} (omp=${{ matrix.omp }},
mpi=${{ matrix.mpi }}, dagmc=${{ matrix.dagmc }}, ncrystal=${{ matrix.ncrystal }},
libmesh=${{ matrix.libmesh }}, event=${{ matrix.event }}
vectfit=${{ matrix.vectfit }})"

This is not quite the same compile arguments that OpenMC CMakelists file obtains. Some of these we don't vary in the CI.

for example the CI matrix does not include
OPENMC_BUILD_TESTS, OPENMC_ENABLE_PROFILE, OPENMC_ENABLE_COVERAGE, OPENMC_USE_UWUW

In addition to the use in CI it is also useful (to myself at least) when locally compiling to know what is enabled and what is not. This PR puts that information into the terminal so the user is reminded of the default values that are used or helping if the user mistypes an arg name they can then check that it is compiled as desired.

@shimwell
Copy link
Member Author

sorry just saw Gridley had already replied.

I see this is approved, thanks for that.

Let us leave this open for a day in case there are any objections

If not I am can to merge this one in tomorrow

@makeclean
Copy link
Contributor

I'm sure you already know, but from your build directory you can also see these options from ccmake ..

@shimwell
Copy link
Member Author

I guess if people have sudo permissions to install cmake-curses-gui and then yes ccmake .. helps with the local build information. So happy to mainly discount that point.

I think the CI usage (original motivation) still stands.

@shimwell
Copy link
Member Author

This will also help with the pip install using scikit core which by builds in a tmp dir and deletes the folder afterwards.
This makes the ccmake option harder to do as the build folder is in the tmp dir and by default deleted.

@shimwell shimwell merged commit 7e033b2 into openmc-dev:develop Feb 11, 2025
16 checks passed
ahnaf-tahmid-chowdhury pushed a commit to ahnaf-tahmid-chowdhury/OpenMC that referenced this pull request Feb 17, 2025
Co-authored-by: Jon Shimwell <jon@proximafusion.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants