Replace internal dependencies by FetchContent

openPMD / openPMD-api

:floppy_disk: C++ & Python API for Scientific I/O

https://openpmd-api.readthedocs.io

GNU Lesser General Public License v3.0

134 stars 51 forks source link

Replace internal dependencies by FetchContent #1583

Open DerNils-git opened 5 months ago

DerNils-git commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

DerNils-git commented 5 months ago

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.

franzpoeschel commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.