-
Notifications
You must be signed in to change notification settings - Fork 287
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
Fix the Android prefab export. #1344
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
Error: You must reformat the sources or regenerate the message map. See the format.patch artifact for more information.
|
{ | ||
// The execution of emscripten cmake script causes the prefab export to fail. | ||
// But here we don't need this execution at all, so we skip it. | ||
if (triplet_file.name == "wasm32-emscripten") continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this isn't the right direction. Triplet names don't carry much meaning. They are identifiers.
Basically, it shouldn't care about any triplet which isn't installed.
Looking at the past issues, I think what is really desired is a user provided positive list of triplets, so that you can pick some architectures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is quite simple, yet it will repair the prefab export, which has been broken for 4 (?) years.
When I think about the command line, I would just omit the triplets entirely.
vcpkg export prefab
or or if you just want to export a specific ABI
vcpkg export prefab --ABI arm64-v8a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which has been broken for 4 (?) years.
A broken fix is not made correct because the bug attempting to be fixed is old.
I agree with @dg0yt that you can't assume anything based on the triplet name. I can make my identical-to-wasm32-emscripten triplet named "fish".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the triplet name issue @dg0yt pointed out this needs end to end tests to verify that it doesn't regress in the future. The reason prefab has been broken so long is that the vcpkg maintainers do not use it and do not know how to validate that it is correct. (I believe it was a community contributed feature, and that we don't have tests for it is a big part of how it was able to become broken)
closes microsoft/vcpkg#35538
closes microsoft/vcpkg#12359