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

[vsg] Update the glslang minimum required version to 15 #42294

Merged
merged 10 commits into from
Nov 27, 2024

Conversation

gwaldron
Copy link
Contributor

@gwaldron gwaldron commented Nov 21, 2024

Updates the glslang minimum required version to the latest version of glslang in vcpkg (15.0.0).
Implemented by passing in an option to vcpkg_cmake_configure().
Failing to do so will prevent vsg from building with glsl compiler support, and will result in a runtime error.

Further information can be found here:
vsg-dev/VulkanSceneGraph#1326

  • 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.

…on of glslang in vcpkg (15.0.0) to prevent vsg from building without glsl compiler support and causing a runtime error
ports/vsg/portfile.cmake Outdated Show resolved Hide resolved
@Cheney-W Cheney-W added the category:port-bug The issue is with a library, which is something the port should already support label Nov 22, 2024
Cheney-W
Cheney-W previously approved these changes Nov 22, 2024
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Nov 22, 2024
@BillyONeal
Copy link
Member

Can you remove the version check in the build entirely rather than hardcoding a particular version from a different port?

@dg0yt
Copy link
Contributor

dg0yt commented Nov 24, 2024

Can you remove the version check in the build entirely rather than hardcoding a particular version from a different port?

#42294 (comment)

@gwaldron
Copy link
Contributor Author

My understanding is that the package maintainer plans to address this in the next release, so I would prefer to defer to them.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 24, 2024

ATM this PR will break the build for users which explicitly select glslang 14 to avoid the problem which the PR wants to fix.

@BillyONeal
Copy link
Member

My understanding is that the package maintainer plans to address this in the next release, so I would prefer to defer to them.

Does that mean we should close this? I think we should either do what we normally do for version-constraints-in-build-systems, that is, patch them out entirely, or do what upstream wants to do. Replacing one hardcoded value with another isn't obviously an improvement to me, particularly if

ATM this PR will break the build for users which explicitly select glslang 14 to avoid the problem which the PR wants to fix.

@BillyONeal BillyONeal marked this pull request as draft November 25, 2024 21:04
@gwaldron
Copy link
Contributor Author

I am going to test your suggestion and if it works I will update the PR

@BillyONeal
Copy link
Member

BillyONeal commented Nov 26, 2024

I am going to test your suggestion and if it works I will update the PR

PR build passed, did it work for you?

@gwaldron
Copy link
Contributor Author

Yes it worked!

@BillyONeal BillyONeal marked this pull request as ready for review November 27, 2024 02:24
@BillyONeal BillyONeal merged commit 1b6d94e into microsoft:master Nov 27, 2024
17 checks passed
@BillyONeal
Copy link
Member

Sounds good, thanks!

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:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants