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

[wasi] Simplify, and clean up target for bundled resource generation #95426

Merged
merged 8 commits into from
Nov 30, 2023

Conversation

radical
Copy link
Member

@radical radical commented Nov 29, 2023

  • [wasi] EmitBuildBase: Output an item for the bundle file also. This is correct as it represents all the outputs
  • [wasi] WasiApp.Native.targets: Simplify target for bundling resources in single file bundle, and remove special cased way of handling bundlefiles
  • [wasi] EmitBundleBase: ReleaseCores once the execution is done, as msbuild does not seem to release them between batched task invocations
  • [wasi] Makefile: add build-packages target

… in single file bundle, and remove special cased way of handling bundlefiles
…build does not seem to release them between batched task invocations
@radical radical added the arch-wasm WebAssembly architecture label Nov 29, 2023
@radical radical requested a review from marek-safar as a code owner November 29, 2023 21:03
@ghost
Copy link

ghost commented Nov 29, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • [wasi] EmitBuildBase: Output an item for the bundle file also. This is correct as it represents all the outputs
  • [wasi] WasiApp.Native.targets: Simplify target for bundling resources in single file bundle, and remove special cased way of handling bundlefiles
  • [wasi] EmitBundleBase: ReleaseCores once the execution is done, as msbuild does not seem to release them between batched task invocations
  • [wasi] Makefile: add build-packages target
Author: radical
Assignees: -
Labels:

arch-wasm

Milestone: -

@radical
Copy link
Member Author

radical commented Nov 29, 2023

@steveisok @mdh1418 Does the addition of the BundleFile in the outputs adversely affect LibraryBuilder, or anything else?

@radical
Copy link
Member Author

radical commented Nov 29, 2023

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mdh1418
Copy link
Member

mdh1418 commented Nov 29, 2023

Does the addition of the BundleFile in the outputs adversely affect LibraryBuilder, or anything else?

I think this could be changed

<_ExtraLibrarySources Include="@(BundledSources)" />
<_ExtraLibrarySources Include="%(_BundledResources.DestinationFile)" />

@(BundledSources) was originally added to include the BundleFile, but if we are emitting BundleFile itself as part of the Outputs, then it will be covered by %(_BundledResources.DestinationFile). (but maybe it wouldn't have duplicates right?)

I think the original intent was for the BundledResources Output parameter to solely be resources that would be processed into the preallocated MonoBundled*Resource structs. As the BundleFile is not actually one of those resources, but instead a separate but necessary file that handles the registration, it would be different from all the other outputs which would have the DataSymbol, DataLenSymbol, and DataLenSymbolValue metadata attached. I think it would be fine to add it, but if you could update the description of the Output property to reflect this, that would helpful. Not sure if adding another Output really makes sense? Or if highlighting that users should include the BundleFile manually makes sense.

@radical
Copy link
Member Author

radical commented Nov 29, 2023

Does the addition of the BundleFile in the outputs adversely affect LibraryBuilder, or anything else?

I think this could be changed

<_ExtraLibrarySources Include="@(BundledSources)" />
<_ExtraLibrarySources Include="%(_BundledResources.DestinationFile)" />

@(BundledSources) was originally added to include the BundleFile, but if we are emitting BundleFile itself as part of the Outputs, then it will be covered by %(_BundledResources.DestinationFile). (but maybe it wouldn't have duplicates right?)
I think the original intent was for the BundledResources Output parameter to solely be resources that would be processed into the preallocated MonoBundled*Resource structs. As the BundleFile is not actually one of those resources, but instead a separate but necessary file that handles the registration, it would be different from all the other outputs which would have the DataSymbol, DataLenSymbol, and DataLenSymbolValue metadata attached. I think it would be fine to add it, but if you could update the description of the Output property to reflect this, that would helpful. Not sure if adding another Output really makes sense? Or if highlighting that users should include the BundleFile manually makes sense.

Hm I'll add a new output like BundleRegistrationFile, which would make it clear that this is also an output, and the caller does not have to "guess" the path of that file. thanks for the suggestion!

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Other than figuring out if we should include the BundleFile in the same Output group as BundledResources or not, looks good to me!

@radical
Copy link
Member Author

radical commented Nov 30, 2023

/azp run runtime-wasm-libtests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Nov 30, 2023

Failures are all known issues.

@radical radical merged commit e839d51 into dotnet:main Nov 30, 2023
173 of 180 checks passed
@radical radical deleted the wasi-emit-bundle-fixes branch November 30, 2023 23:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants