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

Add a hint in the documentation, that the configuration file was created with SameMinorVersion. #11359

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

SunBlack
Copy link
Contributor

Closes #11315 as all other improvements should be implemented on the CMake side.

Is there any Job within the workflow, where I can see the preview of the doc, as I locally don't have Sphinx installed?

@rouault
Copy link
Member

rouault commented Nov 26, 2024

Is there any Job within the workflow, where I can see the preview of the doc, as I locally don't have Sphinx installed?

yes, in the checks there's a "docs/readthedocs.org:gdal" step which is currently in progress. In about 15 to 20 minutes, when it is done, clicking on the "Details" link will lead to the updated rendered docs

@SunBlack SunBlack force-pushed the cmake_SameMinorVersion_hint branch from 93faf7c to 073042d Compare November 26, 2024 15:14
@@ -26,10 +26,27 @@ the cache variable or environment variable ``CMAKE_PREFIX_PATH``. In
particular, CMake will consult (and set) the cache variable
``GDAL_DIR``.

To ensure API compatibility, it is recommended to search for a specific
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the 'recommended' phrasing applies to most use cases. This is rather for people that would want to stick with a particular version. However, in the general case the C API within the 3.x series is backwards compatible, and the C++ one is also close to be backwards compatible for users of the API (a bit less for out-of-tree plugins), so checks of the type GDAL_VERSION VERSION_GREATER_EQUAL "x.y" are what most people will need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that speak in favor of using SameMajorVersion instead of SameMinorVersion? Because you can write find_package(GDAL 3.7.0...3.7.3 REQUIRED), for example, if you are only compatible with GDAL 3.7 (with SameMajorVersion). On the other hand, you cannot write find_package(GDAL 3.5...3.10 REQUIRED) for SameMinorVersion to say that you do support multiple minor versions.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that speak in favor of using SameMajorVersion instead of SameMinorVersion?

yes, if we didn't have those occasional breaks in the C++ plugin API. An out of tree driver might use find_package(GDAL 3.7) to really mean they exactly want 3.7, because 3.8 might break them.
That said they could also write find_package(GDAL 3.7.0...3.7.99) if we do your suggested change

@dg0yt thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would choose SameMajorVersion. TBH in vcpkg we effectively apply SameMajorVersion, although via a wrapper. This probably saves a lot of patching in other packages. But vcpkg doesn't build plugins.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine switching to SameMajorVersion. But we'd need some words of caution regarding breakage in C++ plugin API that might require out-of-tree plugin driver to add more precision version checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, should I then use this PR to customize the readme for 3.10.1 as the behavior is currently and then the customization with SameMajorVersion in a separate PR? Because I think the documentation should also contain the note that there was a change.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, should I then use this PR to customize the readme for 3.10.1 as the behavior is currently and then the customization with SameMajorVersion in a separate PR? Because I think the documentation should also contain the note that there was a change.

if you wish. Not sure which "readme" you refer to exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not readme, I meant the documentaiton I'm touching in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

To be sure of your intentions, do you expect this PR to be backported to the stable branch as well so it appears in the "stable" doc ?
and then you'll do a follow-up PR chaning to SameMajorVersion for master only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since in most cases there are several patch releases per minor release before the next minor release comes out, my idea was to make this PR for the next 3.10 release and then make the CMake changes for 3.11 including the documentation adjustments in a separate PR.

@SunBlack SunBlack force-pushed the cmake_SameMinorVersion_hint branch from 073042d to ebc47a9 Compare December 3, 2024 15:10
@rouault rouault added the backport release/3.10 Backport to release/3.10 branch label Dec 3, 2024
@rouault rouault merged commit dcd8775 into OSGeo:master Dec 3, 2024
3 checks passed
@SunBlack SunBlack deleted the cmake_SameMinorVersion_hint branch December 3, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release/3.10 Backport to release/3.10 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CMake] Version check fails
3 participants