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

[wasm] WasmApp.targets: Separate obj, and bin parts of the build process #47253

Merged
merged 15 commits into from
Jan 25, 2021

Conversation

radical
Copy link
Member

@radical radical commented Jan 21, 2021

We want to use a separate directory for intermediate build outputs, that aren't needed
in the app bundle, and reduce unclear internal dependencies during a build.

TL;DR

Changes:

  1. $(WasmBuildDir) removed
  2. Reasonable defaults set for most properties
  3. To generate a wasm app for a project, the minimum you need to set:
    a. $(WasmMainJS),
    b. and @(WasmAssembliesToBundle)

Details:

Though it is a bit tricky, because the current targets assume:

  • that they are being run after Publish

  • that the publish directory has:

    • some required files copied over from the runtime pack (eg. libmono*),
    • and includes the assemblies
  • that the targets emit all the intermediate output files like driver.o, or the bitcode files, into the same
    directory

  • And there are assumptions about where to pick which files from (eg. whether to take dotnet.wasm
    from the runtime pack, or from the publish dir), based on unclear rules.

What does this PR change?

  • All the assets meant to be from the runtime pack (like libmono*, icudt.dat) are always taken
    only from the runtime pack
    • and this logic is moved out of the tasks, to the targets
  • $(WasmBuildDir) is removed completely. Instead, we use an intermediate path based on $(IntermediateOutputPath)
    • and emit all the intermediate bits there, like the bitcode files
  • Add reasonable defaults for various properties, like $(WasmAppDir)

Effectively:

  1. To generate a wasm app for a project, the minimum you need to set:
    a. $(WasmMainJS),
    b. and @(WasmAssembliesToBundle)

  2. The targets don't depend on publish dir at all
    (in a future PR, we could remove triggering based on Publish target also)

@ghost
Copy link

ghost commented Jan 21, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Generate temporary build artifacts in a separate directory
  • add defaults for most of the properties
Author: radical
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

.. to the targets.

- New property: `NativeAssets`, populated by `@(WasmNativeAsset)`
- Remove property `MicrosoftNetCoreAppRuntimePackRidDir`
- Also, add the `icudt.dat` file from the targets
WasmAppBuilder has (non-obvious) logic to:

1. if AOT'ing, then use the *generated* dotnet.{js,wasm};
2. else use the one from the runtime pack

This depends on Publish having copied those files from the runtime pack
to the publish directory, and then comparing paths in the builder to
decide which one to use.

Instead, make this the intention obvious, and clear.
We were getting these from the publish directory, instead we can get
them directly from the runtime pack.

This includes icudt.dt, and dotnet.timezones.blat .
.. where we can emit the generated native files. Since these files are
meant only for generating the final `dotnet.wasm`, we don't want them to
put them in the bin directory.
.. instead of trying to find them in the build dir. This build directory
will become a directory for intermediate build output in upcoming
commits.
- Instead of having a special $(WasmMainAssemblyPath), and then adding
  it to the wasm assemblies ourselves
  - let the user project add all the relevant assemblies to
    `@(WasmAssembliesToBundle)`, which is usually as simple as
    `$(OutDir)\*.dll`.

- This helps to simplify lot of things.
- And we just need the main assembly filename for generating the
  run-v8.sh script.
Based on the changes in previous commits, we can now remove
`$(WasmBuildDir)`, and replace that with an internal
`$(_WasmIntermediateOutputPath)`. This path will have all the build
artifacts generated that aren't required in the app bundle.

Earlier, we were using the publish directory for that, which resulted in
it being littered with unncessary files, and files getting copied to the
app bundle from unclear sources, and for non-obvious reasons.
@radical radical changed the title [IGNORE][wasm] Improve build [wasm] WasmApp.targets: Separate obj, and bin parts of the build process Jan 22, 2021
@radical radical added the arch-wasm WebAssembly architecture label Jan 22, 2021
@ghost
Copy link

ghost commented Jan 22, 2021

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

Issue Details

We want to use a separate directory for intermediate build outputs, that aren't needed
in the app bundle, and reduce unclear internal dependencies during a build.

TL;DR version:

Changes:

  1. $(WasmBuildDir) removed
  2. Reasonable defaults set for most properties
  3. To generate a wasm app for a project, the minimum you need to set:
    a. $(WasmMainJS),
    b. and @(WasmAssembliesToBundle)

Details:

Though it is a bit tricky, because the current targets assume:

  • that they are being run after Publish

  • that the publish directory has:

    • some required files copied over from the runtime pack (eg. libmono*),
    • and includes the assemblies
  • that the targets emit all the intermediate output files like driver.o, or the bitcode files, into the same
    directory

  • And there are assumptions about where to pick which files from (eg. whether to take dotnet.wasm
    from the runtime pack, or from the publish dir), based on unclear rules.

What does this PR change?

  • All the assets meant to be from the runtime pack (like libmono*, icudt.dat) are always taken
    only from the runtime pack
    • and this logic is moved out of the tasks, to the targets
  • $(WasmBuildDir) is removed completely. Instead, we use an intermediate path based on $(IntermediateOutputPath)
    • and emit all the intermediate bits there, like the bitcode files
  • Add reasonable defaults for various properties, like $(WasmAppDir)

Effectively:

  1. To generate a wasm app for a project, the minimum you need to set:
    a. $(WasmMainJS),
    b. and @(WasmAssembliesToBundle)

  2. The targets don't depend on publish dir at all
    (in a future PR, we could remove triggering based on Publish target also)

Author: radical
Assignees: -
Labels:

arch-wasm, area-Infrastructure-mono

Milestone: -

@radical radical marked this pull request as ready for review January 22, 2021 06:18
@radical
Copy link
Member Author

radical commented Jan 22, 2021

@lambdageek Would you be able to check that this doesn't break your mbr samples?

@vargaz
Copy link
Contributor

vargaz commented Jan 22, 2021

Whats the problem with keeping these in the publish/ dir ? It's not used for actually publishing the app.

@radical
Copy link
Member Author

radical commented Jan 22, 2021

It is cleaner to separate bin/obj. This helps make the internal dependencies clearer. With this PR, the publish dir is used just for the assemblies, so effectively, we can use any directory for that, for eg, after a regular build instead of a full publish.
It will also help with being able to use msbuild clean (more work to be done till we get to that).

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.

Looks good, as long as the mbr sample still works.

@radical
Copy link
Member Author

radical commented Jan 25, 2021

Windows test failures seem to be - #47374 .

@radical radical merged commit b6fb611 into dotnet:master Jan 25, 2021
@radical radical deleted the wasm-obj branch January 25, 2021 22:53
@ghost ghost locked as resolved and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants