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

Use FetchContent_MakeAvailable for asio and eigen #1275

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kaladron
Copy link

CMake 3.30 deprecates FetchContent_Populate as CMP0169. FetchContent_MakeAvailable is already used in tests/CMakeLists.txt so this should not cause any problems for people on older versions of CMake.

Tested:
Built and ran tests using default container with LLVM-19 and CMake 3.30.2 on x86_64 without eigen installed on the image.

@stephenberry
Copy link
Owner

@kaladron, as you can see these changes break the CI tests. Any idea of how to fix this?

@kaladron
Copy link
Author

kaladron commented Sep 4, 2024

Thanks, let me take a look. I only clicked the tests from inside vscode, so I'll try troubleshoot with the same command line

The "clang-linux / build (16, Debug, 23) (pull_request) Failing after 27s" seems to be the image not having f95, but I saw that on some others as well so I think it's not me.

@stephenberry
Copy link
Owner

No, it's not you, I'm pretty sure it is Eigen

CMake 3.30 deprecates FetchContent_Populate as CMP0169.  FetchContent_MakeAvailable is already used in tests/CMakeLists.txt so this should not cause any problems for people on older versions of CMake.

Tested:
Built and ran tests using default container with LLVM-19 and CMake 3.30.2 on x86_64 without eigen installed on the image.
@kaladron
Copy link
Author

kaladron commented Sep 7, 2024

OK, it turns out I was seeing the failures in vscode, vscode just doesn't give ANY notice on ctests failing. Sorry about that. I've adjusted this so that Eigen's tests aren't recursed into. I've used C-f to look for "Not run", and also "failed" to ensure all of them say "0 tests failed".

@stephenberry
Copy link
Owner

Thanks for working on this. There are still some issues with Eigen, as you can see in the Action results:

CMake Error at tests/eigen_test/CMakeLists.txt:15 (set_target_properties):
  set_target_properties can not be used on an ALIAS target.

@kaladron
Copy link
Author

kaladron commented Sep 7, 2024

I think I just need to delete the out/ directory as a test every time before I submit. =( I'm just running tests on a fix.

 - The Eigen documentaiton recommends this for how to use Eigen with CMake packages.  https://eigen.tuxfamily.org/dox/TopicCMakeGuide.html

Set BUILD_TESTING and EIGEN_BUILD_TESTING to off.  Courtesy of the instructions at https://stackoverflow.com/questions/65860094/how-to-add-eigen-library-to-a-cmake-c-project-via-fetchcontent

Tested:
rm -rf out and then exited VSCode, reentered, and did configure, build, and tests from scratch.  In the test window, I don't see rand being run, but I do see eigen_test pass:

[ctest] 1/1 Test stephenberry#5: eigen_test .......................   Passed    0.06 sec
@kaladron
Copy link
Author

kaladron commented Sep 7, 2024

Please take another look. Thank you!

@kaladron
Copy link
Author

kaladron commented Sep 7, 2024

This is driving me nuts. =( It's midnight here, I'll look again in the morning.

@stephenberry
Copy link
Owner

Thanks for pounding your head against this. If you're not able to make progress I can take a look at it. But, I really appreciate you going through this.

@kaladron
Copy link
Author

kaladron commented Sep 8, 2024

No worries. I avoided learning cmake for so long, and I've recently switched to trying to embrace it. It's just frustrating when it builds, you run tests and get all green checkmarks, and then it fails in presubmit! =)

Especially as this is a nothing patch to avoid a warning with a newer minimum cmake. My actual goal is to export a modules interface which only needs cmake 3.28, not the 3.30 I thought - which is only needed for import std. But I'll need to figure this out to submit a bigger patch, so...

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.

2 participants