dart-lang / build

A build system for Dart written in Dart
https://pub.dev/packages/build
BSD 3-Clause "New" or "Revised" License
791 stars 211 forks source link

Fix relative imports in wasm loader #3752

Closed simolus3 closed 2 months ago

simolus3 commented 2 months ago

Currently, the loader script emitted by build_web_compilers resolves URLs against the base-URL of the active document. This breaks when the entrypoint script is not in the same directory as the page, e.g. in

web/
  index.html
  src/
    app.dart

To fix this, this PR changes the logic to resolve URLs against the loader script (which would be /src/app.dart.js in the example, properly resolving the other dart2wasm/dart2js components). I've also added an integration test using a <script> tag pointing to a subdirectory to make sure that this is working now.

Closes https://github.com/dart-lang/build/issues/3751

jakemac53 commented 2 months ago

Will take a look at this early next week, we had an off-site this week

github-actions[bot] commented 2 months ago

Package publishing

Package Version Status Publish tag (post-merge)
package:build_web_compilers 4.1.0-dev ready to publish build_web_compilers-v4.1.0-dev
package:build 2.4.2-wip WIP (no publish necessary)
package:build_config 1.1.2-wip WIP (no publish necessary)
package:build_daemon 4.0.3-wip WIP (no publish necessary)
package:build_modules 5.0.10-wip WIP (no publish necessary)
package:build_resolvers 2.4.3-wip WIP (no publish necessary)
package:build_runner 2.4.13-wip WIP (no publish necessary)
package:build_runner_core 7.3.3-wip WIP (no publish necessary)
package:build_test 2.2.3-wip WIP (no publish necessary)
package:scratch_space 1.0.3-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

github-actions[bot] commented 2 months ago

PR Health

Changelog Entry :heavy_check_mark: | Package | Changed Files | | :--- | :--- | Changes to files need to be [accounted for](https://github.com/dart-lang/ecosystem/wiki/Changelog) in their respective changelogs.
Package publish validation :heavy_check_mark: | Package | Version | Status | | :--- | ---: | :--- | | package:build | 2.4.2-wip | WIP (no publish necessary) | | package:build_config | 1.1.2-wip | WIP (no publish necessary) | | package:build_daemon | 4.0.3-wip | WIP (no publish necessary) | | package:build_modules | 5.0.10-wip | WIP (no publish necessary) | | package:build_resolvers | 2.4.3-wip | WIP (no publish necessary) | | package:build_runner | 2.4.13-wip | WIP (no publish necessary) | | package:build_runner_core | 7.3.3-wip | WIP (no publish necessary) | | package:build_test | 2.2.3-wip | WIP (no publish necessary) | | package:build_web_compilers | 4.1.0-wip | WIP (no publish necessary) | | package:scratch_space | 1.0.3-wip | WIP (no publish necessary) | Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.
simolus3 commented 2 months ago

With the last commit I'm convinced the test failure is probably a flake or something else not caused by me.

test\resolve_source_test.dart: resolveSource can resolve with specified language versions from a PackageConfig (failed)
  PathAccessException: Cannot open file, path = 'D:\a\build\build\build_test\.dart_tool\build_resolvers\sdk.sum' (OS Error: The process cannot access the file because it is being used by another process.
  , errno = 32)
  dart:io                                                 _File.readAsBytesSync
  package:build_resolvers/src/analysis_driver.dart 33:43  analysisDriver
  package:build_resolvers/src/resolver.dart 460:26
jakemac53 commented 2 months ago

With the last commit I'm convinced the test failure is probably a flake or something else not caused by me.

That looks familiar, I believe that when build_resolvers is used by multiple tests it can happen flakily. cc @natebosch do you remember if we had a solution here? I thought we did something

jakemac53 commented 2 months ago

@simolus3 I see there are some legit failures once I deflaked that other failure - some tests with @TestOn('browser') need to exclude wasm it looks like. Or, be re-written to be wasm compatible.

natebosch commented 2 months ago

cc @natebosch do you remember if we had a solution here? I thought we did something

In #3740 we made a change where the file gets written to a separate path and then renamed. This should prevent any situation where the file gets read when it is partially written. It should also prevent any situation where we are trying to read that file when any outside process has it open, which is what this error looks like. The only contention can be from other isolates, or other build_runner instances, and even there the rename should be synchronous.

I don't know if I've seen an error from that particular stack trace before. This might be one we want to keep an eye on and investigate if it's failing frequently.

simolus3 commented 2 months ago

some tests with @TestOn('browser') need to exclude wasm it looks like. Or, be re-written to be wasm compatible.

Only the new subdir_source_test.dart was failing. I think it's because it includes multiple entrypoints (where one is from a subdirectory relative to the test directory, the thing we want to test). But DDC (our probably rather our loader for DDC?) doesn't support multiple entrypoints attached to the same website. We need multiple entrypoints because one is the test (always in the same directory) and another one is in a subdirectory.

simolus3 commented 2 months ago

@jakemac53 Wasm tests on master got broken by https://github.com/dart-lang/sdk/commit/f98a84a3b13cf008c3c636a477b5d7f834d2c9db, I've pushed a fix in d3246e2f0091b168c7d1c389f6589b2f9f601b8d.

kevmoo commented 2 months ago

Holy cow! 🐮 we're green! 💚