Use dart build cli for integration test executable - #4888
Conversation
8cd7afb to
8966a9f
Compare
- Use `dart build cli` to build a standalone pub executable and bundle native assets for integration tests. - Support running binary executable in `test/test_pub.dart`. - Update `tool/test.dart` and CI test workflow to compile using `dart build cli`.
8966a9f to
aaf5234
Compare
Needed for dart-lang/pub#4888. In dart-lang/pub tests, pub was previously precompiled to a snapshot for fast execution. However, snapshot compilation does not support native assets. Switching to `dart build cli` creates a standalone binary, causing `Platform.resolvedExecutable` to point to the standalone executable rather than `dart`. Previously, `FrontendServerClient.start` only used `sdkRoot` as a child compiler argument, but resolved `dartaotruntime` and snapshot binaries relative to `Platform.resolvedExecutable`. This caused compiler spawning failures when running from standalone CLI tools. Also removes dead IA32 code in the scope. TESTED=pkg/frontend_server_client/test/frontend_server_client_test.dart Change-Id: Ib94c15f8fc1cad71b664d0e184a4fd3b90cf96ff Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/542720 Reviewed-by: Ben Konyi <bkonyi@google.com> Commit-Queue: Moritz Sümmermann <mosum@google.com>
| // TODO(sigurdm) To handle signals better we would ideally have `exec` | ||
| // semantics without `fork` for starting the subprocess. | ||
| // https://github.com/dart-lang/sdk/issues/41966. | ||
| final environment = Map<String, String>.from(platform.environment); |
There was a problem hiding this comment.
We need a comment here explaining what is happening, and why platform.resolvedExecutable doesn't work.
Does this work well in the context of dartdev, does that set DART_ROOT? Maybe could use our isRunningFromTest getter here to only do this when testing.
| // one from the sdk. | ||
| final pubInvocation = | ||
| runningFromTest ? platform.script.toFilePath() : 'pub'; | ||
| final String pubInvocation; |
There was a problem hiding this comment.
Again probably better to only change behavior when under test...
| stderr.write('Building pub executable with `dart build cli`...'); | ||
| final buildResult = await Process.run(Platform.resolvedExecutable, [ | ||
| 'build', | ||
| 'cli', |
There was a problem hiding this comment.
I guess build cli is a lot slower than the precompile, have - have you measured the time diff? Currently we can recompile in a few seconds
There was a problem hiding this comment.
[JETSKI AI ANSWER]
Yes, here are the measured timings:
| Command | What it produces | Measured Time |
|---|---|---|
dart --snapshot |
Kernel AST (.dill file) |
~7.9s |
dart build cli (without native assets) |
Full AOT native binary (pub) |
~25.1s |
dart build cli (with package:sigstore) |
Native build hooks + AOT binary | ~42.5s |
dart build cli is slower because it performs full AOT compilation (CFE -> TFA optimization -> gen_snapshot native machine code generation -> ELF linking), plus executes the build.dart hooks for native assets (~16.7s for package:sigstore). In comparison, dart --snapshot only runs the front-end to emit an unoptimized Kernel AST without native code generation or asset bundling.
Full CLI bundling is required for Native Assets (e.g. package:sigstore), but to avoid paying this on every local test run, we could add a freshness check or --no-build flag in tool/test.dart to reuse the existing build when sources haven't changed.
sigurdm
left a comment
sigurdm
left a comment
There was a problem hiding this comment.
Thanks for working on this! A few observations:
- The CI test failures (
Could not find an option named "--sdk-root") are becausefrontend_server_clientresolves the SDK relative toPlatform.resolvedExecutable, which points topubinstead ofdartwhen running as an AOT binary. I saw you landed the fix in the SDK repo (commit 26b1e4c). That package will need to be published to pub.dev andpubspec.yamlupdated here before CI can turn green. masterrecently received workflow fixes (#4900), so this will need a rebase onmaster.- See inline comments regarding binstub generation in production, running tests with assertions enabled, and Windows path quoting.
| } | ||
| } | ||
| } else { | ||
| pubInvocation = 'pub'; |
There was a problem hiding this comment.
Previously the templates had dart $pubInvocation global run, so for production it generated dart pub global run.
With dart removed from the template, pubInvocation = 'pub' generates pub global run (and call pub global run). Since the Dart SDK does not have a standalone pub binary in bin/ (the command is dart pub), fallback binstubs in production will fail with pub: command not found.
Should this be pubInvocation = 'dart pub';?
| } | ||
| return false; | ||
| }(); | ||
| final bool runningFromTest = platform.environment.containsKey('_PUB_TESTING'); |
There was a problem hiding this comment.
Why remove _assertionsEnabled here? dart build cli supports the --enable-asserts flag.
If we compile the test executable with --enable-asserts (in tool/test.dart and .github/workflows/test.yaml), we can retain _assertionsEnabled here and ensure integration tests continue to run with assertions enabled.
| dart %~p0\..\..\bin\pub.dart %* | ||
| ) else ( | ||
| dart %_PUB_TEST_SNAPSHOT% %* | ||
| call %_PUB_TEST_EXECUTABLE% %* |
There was a problem hiding this comment.
Should this be quoted as call "%_PUB_TEST_EXECUTABLE%" %* to prevent breakage if the workspace path contains spaces?
| if (!fileExists( | ||
| p.join(root, 'bin', platform.isWindows ? 'dart.exe' : 'dart'), | ||
| )) { | ||
| environment.remove('DART_ROOT'); |
There was a problem hiding this comment.
When DART_ROOT is set in tests (e.g. pointing to a mock SDK directory without a bin/dart), DartSdk().executable was already initialized to $DART_ROOT/bin/dart. Removing DART_ROOT from environment here won't change the path passed to Process.start, so it attempts to run a nonexistent binary.
2 participants
This PR replaces the legacy
dart --snapshotstep withdart build cli.dart build cli -t bin/pub.dart -o .dart_tool/_pub_buildto create an AOT-compiled CLI bundle for integration tests.package:sigstoredynamic libraries) inbundle/lib/.test/test_pub.dartto support directly executing the precompiled binary.tool/test.dartand GitHub Actions CI workflow to usedart build cli.