Skip to content

[glew] Fix build with CMake 4.0#44086

Merged
JavierMatosD merged 1 commit intomicrosoft:masterfrom
chrismile:glew-cmake-fix
Mar 3, 2025
Merged

[glew] Fix build with CMake 4.0#44086
JavierMatosD merged 1 commit intomicrosoft:masterfrom
chrismile:glew-cmake-fix

Conversation

@chrismile
Copy link
Contributor

@chrismile chrismile commented Feb 28, 2025

This PR fixes the compilation of the GLEW port with CMake 4.0, which removed compatibility with versions of CMake older than version 3.5 (see https://cmake.org/cmake/help/v4.0/release/4.0.html#deprecated-and-removed-features). I have tested on CMake 4.0.0 RC2 that the build process no longer fails with the proposed changes.

While this issue was fixed in the upstream repo some time ago, vcpkg uses the sources of the last release, which has been created a few years ago. portfile.cmake makes this statement why using the code from the main branch of the upstream repo is not directly possible:

# Don't change to vcpkg_from_github! The sources in the git repository (archives) are missing some files that are distributed inside releases.
# More info: https://github.com/nigels-com/glew/issues/31 and https://github.com/nigels-com/glew/issues/13

... so I think the solution proposed by this PR using a patch is the best solution for now.

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 28, 2025

IIUC you could use CMAKE_POLICY_VERSION_MINIMUM instead of adding a patch.
https://cmake.org/cmake/help/v4.0/variable/CMAKE_POLICY_VERSION_MINIMUM.html

@chrismile
Copy link
Contributor Author

IIUC you could use CMAKE_POLICY_VERSION_MINIMUM instead of adding a patch. https://cmake.org/cmake/help/v4.0/variable/CMAKE_POLICY_VERSION_MINIMUM.html

Yes, that should probably also work, but I had the impression adapting cmake_minimum_required would be more consistent with the vcpkg codebase. A search for CMAKE_POLICY_VERSION_MINIMUM in the ports directory gave me no results, while there seems to be many patches with entries for cmake_minimum_required. Some examples for this are:

  • parmetis/build-fixes.patch
  • godot-cpp/packagable.patch
  • ... (in total 27 patches for changing the value used for cmake_minimum_required)

@dg0yt
Copy link
Contributor

dg0yt commented Feb 28, 2025

A search for CMAKE_POLICY_VERSION_MINIMUM in the ports directory gave me no results, while there seems to be many patches with entries for cmake_minimum_required

This variable comes with CMake 4.0, so you can't expect examples.

@chrismile
Copy link
Contributor Author

@dg0yt Feel free to tell me in case I should change using a patch to using -DCMAKE_POLICY_VERSION_MINIMUM=3.5. Probably this will generate a warning when users have older versions of CMake (if I remember correctly, CMake gives a warning when unused/unsupported arguments are detected), but if that's your preferred solution, I'm of course fine with adapting the PR.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 1, 2025

I am a community member. I can review and suggest, but that's all. Because this is the first PR dealing with CMake 4.0, it seems approriate to check all implementation options.

AFAICT it is preferred to avoid patches if possible.
For the warning, there is the MAYBE_UNUSED_VARIABLES option.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 1, 2025

Benefits of the patch:

  • Integration is tested with the current versions of CMake.
  • It is obvious when it becomes obsolete.

So even my tendency is pro patch now.

Comment on lines 9 to 10
-project (glew C)
+project (glew LANGUAGES C)
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyways, this doesn't need a change AFAICT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this was part of the upstream repo commit that changed cmake_minimum_required, so I thought it was necessary, but the old definition is still valid with new CMake versions. I removed it from the patch now.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 3, 2025
@JavierMatosD JavierMatosD merged commit 81e2419 into microsoft:master Mar 3, 2025
18 checks passed
@dg0yt dg0yt mentioned this pull request Mar 4, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-bug The issue is with a library, which is something the port should already support info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants