Skip to content

Fix update_deps.py script to handle Windows Arm64.#1469

Merged
charles-lunarg merged 1 commit into
KhronosGroup:mainfrom
alexrp:main
Apr 15, 2024
Merged

Fix update_deps.py script to handle Windows Arm64.#1469
charles-lunarg merged 1 commit into
KhronosGroup:mainfrom
alexrp:main

Conversation

@alexrp

@alexrp alexrp commented Apr 14, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@ci-tester-lunarg

Copy link
Copy Markdown

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

1 similar comment
@ci-tester-lunarg

Copy link
Copy Markdown

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

@CLAassistant

CLAassistant commented Apr 14, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@alexrp

alexrp commented Apr 14, 2024

Copy link
Copy Markdown
Contributor Author

@anthony-linaro did you run into this while working on #1459? 👀

update_deps.py: error: argument --arch: invalid choice: 'arm64' (choose from '32', '64', 'x86', 'x64', 'win32', 'win64')

Edit: Ah, I guess it's because I'm cross-compiling and passing -A <arch>, so I hit this code, which you probably didn't:

if (CMAKE_GENERATOR_PLATFORM)
list(APPEND update_dep_command "--arch")
list(APPEND update_dep_command "${CMAKE_GENERATOR_PLATFORM}")
endif()

@charles-lunarg charles-lunarg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Normally, I am hesitant to accept any change to update_deps.py because those changes need to be mirrored across all other versions of the script. But, because this is such a procedural change, I am fine with not updating other repo's.

@alexrp

alexrp commented Apr 15, 2024

Copy link
Copy Markdown
Contributor Author

Oh, I didn't realize it was used in other repos. FWIW, if there is a more 'proper' process for submitting changes to this script in a way that gets it mirrored across all relevant repos, I can definitely do that as well.

@charles-lunarg

Copy link
Copy Markdown
Collaborator

The only process is submitting PR's to each repo individually (which is one reason why update_deps.py isn't touched except when it has to be, who likes making the same PR 5+ times?)

@alexrp

alexrp commented Apr 15, 2024

Copy link
Copy Markdown
Contributor Author

Ah. Well, since I'm just messing with it as a one-off, I don't mind sending those PRs as well.

From what I can gather, these are the relevant repos:

Did I miss any? 🤔

Also, for the Vulkan SC repos, is sending a PR actually necessary or are these periodically pulling in changes from the non-SC repos?

@charles-lunarg

Copy link
Copy Markdown
Collaborator

I believe the SC repo's are pulling upstream occasionally, so I wouldn't update them.

Also, I'm going to ping one of the VVL dev's about this script, in case there is some changes they've made that might be incompatible/duplicate with this one.

@ncesario-lunarg Anything in this PR look amiss?

@ncesario-lunarg ncesario-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.

Looking at the diff between VVL's update_deps.py and the loader's, I think this looks good. There is probably some unification that could happen there, but that would be outside the scope of this PR and this change shouldn't break anything.

It is also my recollection that the SC repos will get any changes from here automatically (at least for the validation layers), so those PRs shouldn't be needed.

@charles-lunarg charles-lunarg merged commit 6d9e995 into KhronosGroup:main Apr 15, 2024
@charles-lunarg

Copy link
Copy Markdown
Collaborator

You did miss a repo!
https://github.com/KhronosGroup/glslang (where update_deps.py comes from originally if I understood correctly).

@alexrp

alexrp commented Apr 15, 2024

Copy link
Copy Markdown
Contributor Author

Hmm, I don't see the script in there from a cursory glance. 👀 Does it have a different name there?

@charles-lunarg

Copy link
Copy Markdown
Collaborator

Seems I was mistaken. https://github.com/KhronosGroup/glslang/blob/main/update_glslang_sources.py does download dependencies but doesn't build them, which update_deps.py also handles.

@alexrp

alexrp commented Apr 15, 2024

Copy link
Copy Markdown
Contributor Author

Ok, I think I've sent PRs for all affected repos.

@charles-lunarg

Copy link
Copy Markdown
Collaborator

Thank you for making all of these PR's! Certainly great to know that this change is now in most repo's, should anyone need to build them on arm + windows.

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.

5 participants