-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[browser] Read AssetTraitValue
to get culture for resource during webcil transformation
#89600
Conversation
…cil transformation
Tagging subscribers to 'arch-wasm': @lewing Issue Details
Fixes #89234
|
@@ -70,7 +70,10 @@ public override bool Execute() | |||
var webcilWriter = Microsoft.WebAssembly.Build.Tasks.WebcilConverter.FromPortableExecutable(inputPath: filePath, outputPath: tmpWebcil, logger: Log); | |||
webcilWriter.ConvertToWebcil(); | |||
|
|||
string candicatePath = Path.Combine(OutputPath, candidate.GetMetadata("Culture")); | |||
string candicatePath = candidate.GetMetadata("AssetTraitName") == "Culture" |
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.
Do we have a test that fails without this?
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.
I'm waiting to see it... We have test with resources + Wasm SDK + publish
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.
super nit: candicatePath
-> candidatePath
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.
Where are the paths received here coming from? For satellite assemblies aren't they already in some $culture/$assemblyname.dll
? Why do we need to do this special thing?
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.
This is a task that generates webcil converted libs. It computes a path where to store the intermediate files and using computing unique path for culture specific resource dlls was broken. The metedata comes from static web assets
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 WBT test that I thought it might test the scenario TestAppScenarios.SatelliteLoadingTests
has only a single culture resource and so it works. The other WBT with resources doesn't use Blazor/Wasm SDK
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.
This is what the task is passed:
..../wbt/blz_resources_Debug_ulil2stc.hgf_煉/obj/Debug/net8.0/es-ES/blz_resources_Debug_ulil2stc.hgf_煉.resources.dll
AssetKind=Build
AssetRole=Related
AssetTraitName=Culture
AssetTraitValue=es-ES
Culture=es-ES
RelatedAsset=...../wbt/blz_resources_Debug_ulil2stc.hgf_煉/bin/Debug/net8.0/wwwroot/_framework/blz_resources_Debug_ulil2stc.hgf_煉.dll
RelativePath=_framework/es-ES/blz_resources_Debug_ulil2stc.hgf_煉.resources.dll
TargetPath=es-ES\blz_resources_Debug_ulil2stc.hgf_煉.resources.dll
You can directly use TargetPath, which would respect where rest of the build is expecting the file to be.
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.
And that's why it is needed only in this task, and not in other places.
…cc build on windows
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.
Looks good!
In a follow up PR ConvertDllsToWebCil
should be fixed to not depend on things specific to static webassets, thus it should use the TargetPath
metadata from the candidate
, and not AssetTraitName
.
ReferenceCopyLocalPaths
provideCulture
metadataAssetTraitValue
Fixes #89234