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.

Sign up for GitHub

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

Jump to bottom

CMake: Fix a few minor issues, generate bout-config #2328

Merged
merged 15 commits into from
May 28, 2021
Merged

CMake: Fix a few minor issues, generate bout-config #2328

merged 15 commits into from
May 28, 2021

Conversation

ZedThree
Copy link
Member

  • The implementation of the CONFLICTS argument to bout_add_integrated_test hasn't been merged into next yet, but is used by one of the tests
  • Some of the Find* modules weren't installed
  • The helper scripts weren't installed
  • FindSLEPc always re-runs its tests even if SLEPc has been found
  • FindSUNDIALS didn't cache some of its required variables

Also generates bout-config. This was a bit of a pain to implement due to some of the limitations of CMake, and I basically ended up needing to keep track of the library names manually. This means it's probably a little bit fragile, I've not tested all the possible permutations, but it does at least work in a few configurations on my machine.

There's a "build directory" version, in <build dir>/bin/bout-config, which allows the build directory to be used, as well as a version that gets installed with the correct installation paths.

@ZedThree
CMake: Add CONFLICTS to bout_add_integrated_or_mms_test
e6e02e8 
Disables a test if it conflicts with some variable
@ZedThree
CMake: Install missing cmake modules
ae395bc
@ZedThree
CMake: Provide bout-config
0b062a1
@ZedThree
CMake: Install the various helper scripts
b1dac7a
@ZedThree
CMake: Fix some issues with FindSLEPc module
4e17227 
- wrong syntax for `mark_as_advanced`
- cache package required variables
- explicitly set version variable
- don't rerun the tests if we've already found slepc
@ZedThree
CMake: Cache SUNDIALS variables
ac5d391
@ZedThree ZedThree added the build-system issues with make/configure/... label May 19, 2021
@ZedThree ZedThree added this to the BOUT-5.0 milestone May 19, 2021
@ZedThree ZedThree requested a review from dschwoerer May 19, 2021 12:35
@ZedThree ZedThree mentioned this pull request May 20, 2021
Merged
dschwoerer
dschwoerer reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@dschwoerer dschwoerer left a comment

Choose a reason for hiding this comment

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

I have added a basic test, which is failing in GHA [0].
The same worked before in fedora, so not sure what is wrong. Maybe you have an idea what might be broken?

Before installing the libs line in bout-config is:

libs="-Wl,-rpath,/home/runner/work/BOUT-dev/BOUT-dev/build -L/home/runner/work/BOUT-dev/BOUT-dev/build -lbout++"

The /lib is missing at the end ...

Other things that seem to be wrong:

BOUT_LIB_PATH=
BOUT_CONFIG_FILE=/make.config

[0] https://github.com/boutproject/BOUT-dev/tree/cmake-test-conflicts-test

cmake/BOUT++functions.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@ZedThree
Copy link
Member Author

Before installing the libs line in bout-config is:

libs="-Wl,-rpath,/home/runner/work/BOUT-dev/BOUT-dev/build -L/home/runner/work/BOUT-dev/BOUT-dev/build -lbout++"

The /lib is missing at the end ...

Oops, thanks, looks like I just plain forgot!

Other things that seem to be wrong:

BOUT_LIB_PATH=
BOUT_CONFIG_FILE=/make.config

BOUT_LIB_PATH is probably not needed if we're using the full path anyway, but might make it easier to modify in future? Not sure.

BOUT_CONFIG_FILE is also probably not needed in any case? If you're using bout-config to build, then you don't need make.config, and vice versa. I'm not certain we can provide a sensible version of make.config using CMake -- getting bout-config working was a fair bit of effort already.

@dschwoerer
Copy link
Contributor

I think providing a make.config would be nice, as that would allow to transition easily from 4 to 5, at least in terms of build tools.

I think it should be doable to just rely on bout-config for that, I can have a look 👍

Anyway, that is nothing to block this PR, having bout-config certainly is very nice, although I expect there will be some cases that don't work yet, but nothing that cannot be fixed later easily ...

dschwoerer
dschwoerer previously approved these changes May 27, 2021
View reviewed changes
@ZedThree
Copy link
Member Author

Good point, it would definitely be nice to provide make.config. Thanks!

@ZedThree ZedThree requested a review from dschwoerer May 27, 2021 08:36
@ZedThree ZedThree merged commit 3383422 into next May 28, 2021
@ZedThree ZedThree deleted the cmake-test-conflicts branch May 28, 2021 08:56
@ZedThree ZedThree mentioned this pull request Jul 2, 2021
Closed
6 tasks
@ZedThree ZedThree mentioned this pull request Jul 29, 2021
Merged
@dschwoerer dschwoerer mentioned this pull request Oct 19, 2022
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschwoerer dschwoerer dschwoerer approved these changes

Assignees
No one assigned
Labels
build-system issues with make/configure/...
Projects
None yet
Milestone
BOUT-5.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ZedThree @dschwoerer