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

build: add cmake build for FDS #13968

Merged
merged 1 commit into from
Jan 3, 2025
Merged

Conversation

JakeOShannessy
Copy link
Contributor

@JakeOShannessy JakeOShannessy commented Jan 1, 2025

This PR adds a CMake build for FDS.

With the addition of third-party libraries there are now a few more steps involved in building FDS. The scripts help with this but they necessarily make a few assumptions, including the setup of a the users home directory.

CMake is a useful alternative to this, particularly as it is used by both sundials and hypre, and will likely be used by any other libraries in this space.

The current CMakeLists.txt handles all the current platforms contained in Build/ and can either use sundials and hypre installed in the system or handle the download and build of those libraries directly from GitHub. A few different configuration options are provided at the start of CMakeLists.txt.

I build RPMs for rockylinux and fedora and have found this a more reliable solution. The two build systems can coexist, providing for CMake is simply adding this single file.

To build, run the following:

cmake -B build-dir -S .
cmake --build build-dir

This will result in build-dir/fds on most platforms.

Options can be appended in the following way (e.g., to turn off hypre):

cmake -B build-dir -S . -DUSE_SUNDIALS=OFF -DUSE_HYPRE=OFF
cmake --build build-dir

P.S.: Currently CMakeLists.txt points to a forked version of hypre due to HYPRE_FMANGLE. Hopefully a PR gets merged to resolve this.

Adds a CMake build file. This builds FDS for all supported platforms and
compilers. It also handles fetching and building the third-party
libraries (currently sundials and hypre) where system-installed versions
are not used.

The CMake build is tested on most of the configurations in Build/.
@JakeOShannessy JakeOShannessy marked this pull request as ready for review January 2, 2025 07:43
@mcgratta
Copy link
Contributor

mcgratta commented Jan 2, 2025

Thanks, we will take a look at this.

@rmcdermo rmcdermo merged commit e5bc376 into firemodels:master Jan 3, 2025
33 checks passed
@rmcdermo
Copy link
Contributor

rmcdermo commented Jan 3, 2025

@JakeOShannessy Thanks! When I tested the PR I a saw a lot of warnings about a change in Ubuntu version here:

actions/runner-images#10636

I assume these are innocuous at the moment. But if we can get ahead of this breaking anything, that would be great. Thanks again!

@rmcdermo
Copy link
Contributor

rmcdermo commented Jan 3, 2025

@JakeOShannessy Is it is standard for CMakeLists.txt to be at the top level of the project? If so, that's fine. But if this is flexible, can we move it to be under fds/Build/?

@JakeOShannessy
Copy link
Contributor Author

@JakeOShannessy Thanks! When I tested the PR I a saw a lot of warnings about a change in Ubuntu version here:

actions/runner-images#10636

I assume these are innocuous at the moment. But if we can get ahead of this breaking anything, that would be great. Thanks again!

I think they are innocuous but it's a good point so I've switched the images ahead of time. The patch I submitted to hypre has been merged (with subtle changes) so I'll make a few updates and send in a PR.

@JakeOShannessy Is it is standard for CMakeLists.txt to be at the top level of the project? If so, that's fine. But if this is flexible, can we move it to be under fds/Build/?

It's definitely the most common to have it at the top level but it's not a hard requirement. It will certainly function but may have an impact on how well it integrates with IDEs and the like so I'll test it.

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