Skip to content

Update deps enhancements#8705

Merged
spencer-lunarg merged 2 commits into
KhronosGroup:mainfrom
RasterGrid:update-deps-enhancements
Oct 16, 2024
Merged

Update deps enhancements#8705
spencer-lunarg merged 2 commits into
KhronosGroup:mainfrom
RasterGrid:update-deps-enhancements

Conversation

@MathiasMagnus

Copy link
Copy Markdown
Contributor

This PR contains fixes to two annoyances of ours, both arguably bug candidates:

  • Dependency fetching breaks when the remote URL is changed in update_deps.py, which can easily be handled by wiping the clone dir.
  • Both documentation and code suggests that the XYZ_INSTALL_DIR variables can be used to override where dependencies should be taken from, but the dep fetching machinery outright overwrites all user-provided paths.

@MathiasMagnus MathiasMagnus requested a review from a team as a code owner October 16, 2024 11:51
@ci-tester-lunarg

Copy link
Copy Markdown
Collaborator

Author MathiasMagnus not on autobuild list. Waiting for curator authorization before starting CI build.

@MathiasMagnus MathiasMagnus force-pushed the update-deps-enhancements branch from 1c2e841 to 7bc5fb3 Compare October 16, 2024 11:52
@ci-tester-lunarg

Copy link
Copy Markdown
Collaborator

Author MathiasMagnus not on autobuild list. Waiting for curator authorization before starting CI build.

@MathiasMagnus MathiasMagnus force-pushed the update-deps-enhancements branch from 7bc5fb3 to 69638b4 Compare October 16, 2024 11:56
@ci-tester-lunarg

Copy link
Copy Markdown
Collaborator

Author MathiasMagnus not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg

Copy link
Copy Markdown
Collaborator

Author MathiasMagnus not on autobuild list. Waiting for curator authorization before starting CI build.

Without going through an intricate dance with git to change the remote
URL of a repository, it's simplest to just nuke the cloned folder, as
we have to do a clean clone anyway.
The documentation states that users can override where to take a
dependency from via variables like VULKAN_HEADERS_INSTALL_DIR, however
the helper.cmake written by the scripts/CMakeLists.txt FORCE updates
the cache variable to the location inside known_good.json, practically
overwriting the user-provided path given on the CLI using -D, also
going into the cache.
@MathiasMagnus MathiasMagnus force-pushed the update-deps-enhancements branch from 4b399d8 to a947b79 Compare October 16, 2024 11:59
@ci-tester-lunarg

Copy link
Copy Markdown
Collaborator

Author MathiasMagnus not on autobuild list. Waiting for curator authorization before starting CI build.

@MathiasMagnus

Copy link
Copy Markdown
Contributor Author

By the way, has LunarG considered externalizing all the build related utilities to a dedicated repository? This way it would be easier to propagate changes to all the repositories, but the biggest gain would be to have a dedicated place where the utility scripts could be tested (for eg. if the repo URL is changed, can one rerun update_deps.py? Other ecosystems have profited greatly from such externalized collection of helpers, such as rocm-cmake that all ROCm components use to build in a uniform style.

@spencer-lunarg spencer-lunarg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@charles-lunarg any objections?

something to be aware of is unfortunately this update_deps.py is a example of copy-and-paste gone wrong and you may want to upstream fixes everywhere else

KhronosGroup/Vulkan-Loader#1469 (comment)

@spencer-lunarg

Copy link
Copy Markdown
Contributor

By the way, has LunarG considered externalizing all the build related utilities to a dedicated repository?

Yes, we have considered and very aware of the problem, I think the issue is just having someone spend the engineering hours going around and updating things and making sure they break everyone as they are doing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants