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

Replace internal dependencies by FetchContent #1583

Merged
merged 34 commits into from
Sep 5, 2024

Conversation

DerNils-git
Copy link
Contributor

@DerNils-git DerNils-git commented Jan 6, 2024

Replace internal dependencies management by CMake FetchContent commands.

Advantage:

  • Updating the dependencies is simply done by updating the version/hash used within FetchContent_Declare which makes it easier to stay at the project HEAD of dependencies for project maintainers
  • Less CMake options that users have to take care about while keeping the same flexibility

Required changes:

  • Increase CMake minimum required version

Dependencies still have to be updated:

  • TOML11 requires CMAKE_CXX_STANDARD to be set if the latest hash shall be used
  • Catch2 requires to be moved from v2 to v3 which requires additional source changes.

If the pull request is accepted I would open issues to update the dependencies mentioned above and assign them to me.

(Sorry about the slightly chaotic git history. The change to FetchContent is done from 0ba14dc to 0084e9b. The previous commits resulted from a previous try to move to FetchContent which failed and the commits are reverted)

DerNils-git and others added 20 commits May 24, 2023 10:09
…D interface library for nlohmann_json"

This reverts commit 6136a97.
Add the directory filled by FetchContent to .gitignore
Add CMakeUserPresets.json to .gitignore
.gitignore Outdated
share/openPMD/thirdParty/json/
share/openPMD/thirdParty/toml11/
share/openPMD/thirdParty/catch2/
share/openPMD/thirdParty/pybind11/
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be aware that with this, the build folder will modify the source tree.
What does the workflow look like switching between different openPMD versions that specify different dependency versions. Will FetchContent notice and update the dependencies' versions upon configuring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true and might be a problem. I don't have experience with this.

Moving from lower to higher versions of OpenPMD should not be a problem. These directories have been deleted using git rm and should therefore be deleted when moving forward. FetchContent can then download into these directories w/o problem.

A problem might arise if one moves from a higher to a lower version. If the directories have been populated by FetchContent and git tries to add the old libraries again in the same directories I can not predict the behavior.

It would be possible to use a different directory with FetchContent until we are sure that it works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since @ax3l uses FetchContent in WarpX, let's wait until he is back to see what their approach looks like (relevant bits in their CMake config: https://github.com/ECP-WarpX/WarpX/blob/11e2a1722aac8929db0ed677f634673d235c1396/cmake/dependencies/openPMD.cmake#L29)

Copy link
Member

Choose a reason for hiding this comment

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

I'll move this to the build directory, as we do in other project.

CMakeLists.txt Outdated
toml11
GIT_REPOSITORY https://github.com/ToruNiina/toml11
# Migrate to the latest commit to remove CMake Warning which is not yet
# available in any official release.
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the warning below? Do I understand correctly that this has been fixed on toml11 dev and we are waiting for the next release?

CMake Deprecation Warning at share/openPMD/thirdParty/toml11/CMakeLists.txt:1 (cmake_minimum_required):                                                                                                                    
  Compatibility with CMake < 3.5 will be removed from a future version of                                                                                                                                                  
  CMake.                                                                                                                                                                                                                   
                                                                                                                                                                                                                           
  Update the VERSION argument <min> value or use a ...<max> suffix to tell                                                                                                                                                 
  CMake that the project does not need compatibility with older versions. 

Copy link
Contributor Author

@DerNils-git DerNils-git Jan 16, 2024

Choose a reason for hiding this comment

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

Yes.
But also moving to the last commit is not trivial with toml11 since that requires CMAKE_CXX_STANDARD to be set which I could not add to openPMD-api w/o breaking the CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

since that requires CMAKE_CXX_STANDARD to be set which I could not add to openPMD-api w/o breaking the CI.

That's probably an issue unrelated to this PR which we should better solve separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, after finishing this PR I would try to Update catch2 and toml11 with separate issues as described above

CMakeLists.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@DerNils-git
Copy link
Contributor Author

Is it to be expected that one of the CI stages fails? The error in this step does not seem to be related to the changes of this pull request.

@franzpoeschel
Copy link
Contributor

Is it to be expected that one of the CI stages fails? The error in this step does not seem to be related to the changes of this pull request.

The failure comes from the increased minimum CMake version:

CMake Error at CMakeLists.txt:3 (cmake_minimum_required):
  CMake 3.24.0 or higher is required.  You are running version 3.22.1

The other "red text" seems to be unrelated (I see it on other PRs also), but does not cause a failure

Is version 3.22 also fine as a minimum? Otherwise, I'll push a commit that tries bumping the minimum CMake version of that CI run.

@DerNils-git
Copy link
Contributor Author

Yes, sorry I was confused by the first red section in that action.

CMake 3.24 was the lowest version which still works with the implementation that I provided here. Using a lower CMake Version requires to remove the OVERIDE_FIND_PACKAGE argument in FetchContent.

@franzpoeschel
Copy link
Contributor

It seems like we need to consider saying good-bye to the Win32 CI runner. The last updates in https://repo.anaconda.com/pkgs/main/win-32/ were in 2022, the latest included CMake version is 3.22.1 from Dec 7, 2021, hence the failure.

@franzpoeschel
Copy link
Contributor

Using a lower CMake Version requires to remove the OVERIDE_FIND_PACKAGE argument in FetchContent.

After an offline discussion, this might be the preferred way to go. Even if Win32 is end of life, the runners are still useful as they are a good way to check our handling of datatypes, also Win32 is still being used in lab settings.
Given that this is a program library. we should be careful about raising the required CMake version to something released only very recently, if it can be avoided.

Revert previous commit to use cmake 3.22 in Win32 CI runner.
@franzpoeschel
Copy link
Contributor

Is it possible to trigger the failed CI again. In the last run it seems that there was an internal error since the directory in which openPMD should have been placed already existed.

I think the Appveyor part of our CI has no way to trigger a selective restart
I force-pushed to give it a second chance

@DerNils-git
Copy link
Contributor Author

@ax3l is there any chance to get direct access to the system on which the CI is failing? That would be easier than debugging this error by additional commits to this pull request.
Unfortunately I could not reproduce this error locally.

@franzpoeschel
Copy link
Contributor

I can reproduce the same error an a Linux system using CMake 3.22.1
This might be easier for investigating, I'll have a look

@franzpoeschel
Copy link
Contributor

The problem is that CMake 3.22 does not correctly interpret the SOURCE_DIR param on FetchContent_Declare, leaving empty directories. This leads ot a rather cryptic failure in FetchContent_MakeAvailable.
Seems fixed with CMake 3.23. I'd say, we can either use the SOURCE_DIR param only in CMake 3.23 or later, alternatively leave it out altogether.

@DerNils-git
Copy link
Contributor Author

We could also just depricate the SOURCE_DIR keyword. I hoped that FetchContent skips the download if the directory is already populated. Unfortunately this does not work. Therefore adding the SOURCE_DIR does not give any advantage and we could also just use the CMake default directories for FetchContent.

@ax3l ax3l self-assigned this Jan 30, 2024
@ax3l ax3l added this to the 0.16.0 milestone Jan 30, 2024
@ax3l ax3l added the install label Jan 30, 2024
@franzpoeschel franzpoeschel mentioned this pull request Jul 11, 2024
3 tasks
@ax3l ax3l force-pushed the add_cmake_fetchContent_new branch 2 times, most recently from 580f5ff to 289c872 Compare September 5, 2024 20:35
Dependent projects might have the same dependencies and build
as a superbuild as well.
@ax3l
Copy link
Member

ax3l commented Sep 5, 2024

Thanks a lot, @DerNils-git!

I'll merge this and do a few follow-up PRs:

One thing I really miss in FetchContent is that it seems to lack support for shallow git clones, which can be a major time sink for large repos. If someone finds an option for this please let me know and we will add it (aka git clone --depth 1).

Alternatively, we could pull a tarball from the releases page of projects... #1668
https://cmake.org/cmake/help/latest/module/FetchContent.html#overview

@ax3l ax3l merged commit 570f155 into openPMD:dev Sep 5, 2024
31 checks passed
@ax3l ax3l mentioned this pull request Sep 17, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants