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

Bump minimum CMake to 3.19 and introduce version ranges #2358

Merged
merged 24 commits into from
Dec 21, 2022

Conversation

jorgensd
Copy link
Sponsor Member

@jorgensd jorgensd commented Sep 9, 2022

Cmake 3.19 added version ranges: https://cmake.org/cmake/help/latest/release/3.19.html
back in november 2020. This would make it easier for DOLFINx extensions to specify requirements on DOLFINx.

For instance:
v0.5.1 was just released with some bug fixes, and no changes in the API.
I need to rewrite my CMakeLists.txt for dolfinx_mpc to support this change. With version ranges, I would be able to specify

find_package(DOLFINX 0.5...0.6 REQUIRED)

which would result in:

-- The following REQUIRED packages have been found:
 * DOLFINX (required version 0.5...0.6), New generation Dynamic Object-oriented Library for - FINite element computation, <https://github.com/FEniCS/dolfinx>

when using v0.5.1.
For more rules regarding ranges see:
https://cmake.org/cmake/help/latest/command/find_package.html#basic-signature

@jorgensd jorgensd added enhancement New feature or request build Build system and compiler issues labels Sep 9, 2022
@jorgensd jorgensd changed the title Bump minimum CMake to 3.19 and introduce versions ranges Bump minimum CMake to 3.19 and introduce version ranges Sep 9, 2022
@jorgensd
Copy link
Sponsor Member Author

jorgensd commented Sep 9, 2022

@drew-parsons I think this will help when bumping different parts of the FEniCS eco-system, making it easier to provide minimal bounds (If we introduce this in Basix as well).

@drew-parsons
Copy link
Contributor

This is probably safe. Would want to check conda support but almost certainly their cmake is up to date.

Ubuntu 20.04 focal has cmake 3.16. But we're already not able to (simply) build dolfinx 0.5 because of the use of C++20 features (std::span), not supported by gcc-9. So for that reason ubuntu 20.04 support is not an impediment (it will have to be left on dolfinx 0.4).

Might want to consider if we're happy with that ubuntu situation, or whether people building dolfinx manually on ubuntu 20.04 should still be supported. The policy is on a best-effort basis, so can instead just ask such users to upgrade to a more recent ubuntu release.

@francesco-ballarin might have an opinion on dropping old ubuntu support, because of the Google colab fiasco.

@francesco-ballarin
Copy link
Member

francesco-ballarin commented Sep 9, 2022

@jorgensd @drew-parsons I am fine with dropping support for ubuntu 20.04, my FEM on Colab pipeline is very mildly dependent on upstream ubuntu packages: due (as you call it) to that fiasco I basically have to rebuild and package any dependency anyway in my own workflow.

I've just checked:

  • the cmake version in my workflow is 3.24.1 (i.e., the latest from kitware apt), so well above 3.19. Therefore, requiring 3.19 will not hinder my workflow.
  • the cmake version of an actual Colab instance is 3.22.6 (as of today, might change once in a while), so again above 3.19. Can you please remind me if JIT compilation actually uses cmake? I seem to remember that it was the case in dijitso, but if I remember correctly now JIT uses pkgconfig (and cppimport?) and never cmake. If that was the case, the end user would never have to use cmake on an actual Colab instance, unless they are doing something crazy (like deciding to compile the C++ demos on Colab, but what would be the point? ;) )

Copy link
Member

@jhale jhale left a comment

Choose a reason for hiding this comment

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

It would also be good if DOLFINx explicitly specified it's dependency on UFCx through ranges rather than implicitly through its own version number.

@jorgensd
Copy link
Sponsor Member Author

It would also be good if DOLFINx explicitly specified it's dependency on UFCx through ranges rather than implicitly through its own version number.

PR now coupled with: FEniCS/ffcx#535

I guess we want something similar in BASIX (we currently have no version requirement of basix, which I guess is wrong)? @garth-wells @mscroggs

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -147,7 +147,7 @@ install(FILES ${CMAKE_CURRENT_BINARY_DIR}/common/version.h DESTINATION ${CMAKE_I
include(CMakePackageConfigHelpers)
write_basic_package_version_file(${CMAKE_BINARY_DIR}/dolfinx/DOLFINXConfigVersion.cmake
VERSION ${DOLFINX_VERSION}
COMPATIBILITY ExactVersion)
COMPATIBILITY AnyNewerVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Why not same minor version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jack, COMPATIBILITY SameMinorVersion is I think more appropriate, at least at this stage in dolfinx' development. Otherwise 0.6.0 would be considered compatible with 0.5.2.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

That does not work with the version ranges unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does SameMinorVersion give the same problem as ExactVersion? I would have expected it to allow any patch version with the same major.minor. If it doesn't work that way then fair enough. If AnyNewerVersion is used here, then it would be possible to exclude 0.6 in the client program by using ...<0.6 with the find_package version.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I will test it again tomorrow, but as far as I can recall it had the same issues. I would say it is up to the developer of the other package (me in the case of DOLFINx-MPC) to set sensible ranges in find_package in my cmake file, similarly to how you would pin, or set version ranges in Python packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Likely you already thought of it, but one thought in regards to testing SameMinorVersion, both packages (dolfinx and dolfinx_mpc) would need cmake_minimum_required(VERSION 3.19). i.e. dolfinx_mpc would also need that updated alongside the version range.

@jorgensd
Copy link
Sponsor Member Author

jorgensd commented Nov 1, 2022

Summary of cmake:
Assume that we have DOLFINx 0.5.2.0 installed with

write_basic_package_version_file(${CMAKE_BINARY_DIR}/dolfinx/DOLFINXConfigVersion.cmake
  VERSION ${DOLFINX_VERSION}
  COMPATIBILITY AnyNewerVersion)

in /cpp/dolfinx/CMakeLists.txt.
Then:

  1. find_package(DOLFINX 0.4.0 REQUIRED) works, as we accept any newer version
  2. find_package(DOLFINX 0.5.3 REQUIRED) leads to an error, as we can only find version 0.5.2.0
  3. find_package(DOLFINX 0.6.0 REQUIRED) leads to the same error
  4. find_package(DOLFINX 0.5...0.6 REQUIRED) works
  5. find_package(DOLFINX 0.5.0...<0.6.0 REQUIRED) works
  6. find_package(DOLFINX 0.4.0...<0.5.0 REQUIRED) throws an error
  7. find_package(DOLFINX 0.4.0...0.5.0 REQUIRED) throws an error

Point 1. can be thought of as a lower bound on the version you can use, similar to how one usually set version requirements in Python.
i.e.
using find_package(0.4.0) is equal to version >= 0.4.0
if you want an upper bound, say within the same minor version, you would need to explicitly hard-code it (which at least from a pythonic viewpoint is sensible).

Copy link
Member

@jhale jhale left a comment

Choose a reason for hiding this comment

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

I'd like to have this merged in for a bit of in-the-wild testing prior to the 0.6.0 release.

@drew-parsons
Copy link
Contributor

I've just hit this problem in practice while trying to package @jorgensd 's dolfinx-mpi for debian. ExactVersion in cpp/dolfinx/CMakeLists.txt means that dolfinx 0.5.2 is not compatible with the dolfinx 0.5.1 requested by dolfinx-mpc, even when patched with a version range 0.5.1...<0.6 (note the < should be there, or else 0.6.0 itself would be considered compatible)

I think the patch to cpp/dolfinx/CMakeLists.txt should declare COMPATIBILITY SameMinorVersion not COMPATIBILITY AnyNewerVersion, though. I am assuming here that dolfinx 0.6 is ABI incompatible with 0.5.

The changes to the .yml files would need to be reverted before merging this PR.

@jorgensd
Copy link
Sponsor Member Author

SameMinorVersion

@drew-parsons If we run with SameMinorVersion, one would force the user to use:

find_package(DOLFINX 0.6.0...<0.7.0 REQUIRED)

I.e. something like:

find_package(DOLFINX 0.6.0...0.7.0 REQUIRED)

is not accepted and throws:

CMake Error at CMakeLists.txt:93 (find_package):
  Could not find a configuration file for package "DOLFINX" that is
  compatible with requested version range "0.6.0...0.7.0".

  The following configuration files were considered but not accepted:

    /usr/local/dolfinx-real/lib/cmake/dolfinx/DOLFINXConfig.cmake, version: 0.6.0.0

I find it very strange that we force people to bump the version of their code whenever we release a version of DOLFINx.
I would say it is up to the developer of the package depending on DOLFINx to make sure that their code is compatible with the latest release. There might be cases where DOLFINx has API changes, but they are not affecting the external package.
@jhale @garth-wells @chrisrichardson your thoughts on this?

@drew-parsons
Copy link
Contributor

drew-parsons commented Dec 19, 2022

I find it very strange that we force people to bump the version of their code whenever we release a version of DOLFINx.

You're absolutely right, this is the key point. Will the API be substantially changing in every 0.x release, requiring users to modify their existing code? Across 0.4 to 0.5, and 0.6, that has been the case, and I was proposing we'd want the 0.6...<0.7 variant (with SameMinorVersion), on the assumption that 0.7 would also introduce API changes.

At some point the API changes would slow down, at which point it would be time to make a 1.0 release. At that point the policy could change to SameMajorVersion assuming semantic versioning is employed. But I'd still argue then for SameMajorVersion rather than AnyNewerVersion.

If no major API changes are expected, but new major versions are wanted anyway (not semantic versioning), or if we want to just leave it to users to manage version compatibility themselves, then AnyNewerVersion is fine.

@jorgensd
Copy link
Sponsor Member Author

You're absolutely right, this is the key point. Will the API be substantially changing in every 0.x release, requiring users to modify their existing code? Across 0.4 to 0.5, and 0.6, that has been the case, and I was proposing we'd want the 0..6...<0.7 variant (with SameMinorVersion), on the assumption that 0.7 would also introduce API changes.

Im not sure we should enforce this on the user. Some of the API changes every minor release, but it doesn’t necessarily mean that it will break every package based on DOLFINx. I guess we need to decide something. What do you think @jhale ?

@jhale
Copy link
Member

jhale commented Dec 20, 2022

My understanding of the discussions so far is that we aim to use Semantic Versioning 2.0. That means that public API changes will automatically lead to a major version bump once we go past v1.0.0. At the moment, pre-v1.0.0 even minor version bumps signify a public API change.

Whether an API change actually breaks a downstream package is not important (e.g. they only use a limited subset of the API). It is for downstream users and developers to test and make that determination.

https://semver.org

@jorgensd
Copy link
Sponsor Member Author

Whether an API change actually breaks a downstream package is not important (e.g. they only use a limited subset of the API). It is for downstream users and developers to test and make that determination.

If we want to go that route, I would suggest we use AnyNewerVersion, as that makes it up to the developer of the corresponding C++ library to set the correct version range.

@jhale
Copy link
Member

jhale commented Dec 20, 2022

P.S. I don't see why downstream developers should be in anyway tied into to DOLFINx's version numbering.

If I want to have my downstream package at version 0.1 and it's compatible with DOLFINx 0.5...<0.7 then I should be able to do it?

@jorgensd jorgensd mentioned this pull request Dec 20, 2022
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.github/workflows/ccpp.yml Outdated Show resolved Hide resolved
.github/workflows/ccpp.yml Outdated Show resolved Hide resolved
@jorgensd jorgensd merged commit 7d2e020 into main Dec 21, 2022
@jorgensd jorgensd deleted the dokken/cmake_version_range branch December 21, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system and compiler issues enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants