Skip to content

[native_assets_cli] Protocol exit codes / unhandled exceptions #33

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.

Sign up for GitHub

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

Jump to bottom
Closed
dcharkes opened this issue May 3, 2023 · 3 comments · Fixed by #1762
Closed

[native_assets_cli] Protocol exit codes / unhandled exceptions #33

dcharkes opened this issue May 3, 2023 · 3 comments · Fixed by #1762
Assignees
@dcharkes
Labels
P2 A bug or feature request we're likely to work on package:hooks_runner package:hooks
Milestone
Native Assets v1.0

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented May 3, 2023 •
edited
Loading

I'm leaning towards specifying hooks will always be run as a separate process, and non-zero exit code means error.

We need to specify whether build.dart is always run as a separate process.
And we need to specify the failure modes for a build. (e.g. 0-zero exit code for an unhandled exception.)
E.g. what happens if a build succeeds at first, and then fails later. Who is responsible for deleting the outdated build_output.yaml? (The Dart / Flutter SDK.)

We could consider using Isolate.spawnUri to invoke build.dart scripts rather than Process.run.

  • This would mean build.dart scripts should not call exit(int).
    It would probably also mean we need to think about what to do with unhandled exceptions.
  • Doing this would isolate the builds less.
  • Doing this might speed up the builds slightly (we'd still need to JIT from source every time, so we'd only be saving the Dart VM C++ process startup)

I believe it's probably best to use process isolation for the builds and not run them as Isolate.spawnUri
@dcharkes dcharkes added the P2 A bug or feature request we're likely to work on label May 3, 2023
@dcharkes dcharkes mentioned this issue May 3, 2023
Closed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue May 15, 2023
@dcharkes
[pkg][vm] Native Assets builder
d360edf 
This package contains the logic for building native assets.

This package is the backend that invokes toplevel `build.dart` scripts.
For more info on these scripts see https://github.com/dart-lang/native.

This is a separate package so that dartdev and flutter_tools can reuse
the same logic without flutter_tools having to import dartdev.

Some design decisions:

* We don't yet have `build_dependencies`, so we use the ordinary
  dependency graph for ordering of native assets builds. (If there is
  a cycle we refuse to run.)
  Bug: dart-lang/pub#3794
* Builds are cached based on all the configuration provided by the
  caller. Environment variables are ignored in caching. This CL also
  contains a unit test that invokes the build by not passing through
  environment variables. However, for Windows we need to pass through
  at least `SYSTEMROOT` for MSVC to run correctly. So we might need
  to further explore if we can/want to lock env variables down.
  Bug: dart-lang/native#32
  Bug: dart-lang/native#33

Run tests:
```
dart tools/generate_package_config.dart && \
tools/test.py -n unittest-asserts-release-linux pkg/native_assets_builder
```

Bug: #50565
Change-Id: I133052d7195373e87d20924d61e1e96e3d34ce8f
Cq-Include-Trybots: luci.dart.try:pkg-linux-debug-try,pkg-linux-release-try,pkg-mac-release-arm64-try,pkg-mac-release-try,pkg-win-release-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/300203
Reviewed-by: Liam Appelbe <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Hossein Yousefi <[email protected]>
@dcharkes
Copy link
Collaborator Author

Also, when addressing this, we should see that the user experience is good when one package build fails when there are multiple packages with native assets.

This was referenced Aug 1, 2023
Closed
Closed
@dcharkes dcharkes added this to the Native Assets v1.0 milestone Aug 30, 2024
@dcharkes
Copy link
Collaborator Author

Also, when addressing this, we should see that the user experience is good when one package build fails when there are multiple packages with native assets.

Currently it's broken:

@mkustermann
Copy link
Member

Currently it's broken:

Better to verify, but I may have fixed that as well in 5164777

@dcharkes dcharkes self-assigned this Nov 28, 2024
@dcharkes dcharkes mentioned this issue Nov 28, 2024
Merged
@dcharkes dcharkes moved this to In Progress in Native Assets Nov 28, 2024
auto-submit bot pushed a commit that referenced this issue Nov 28, 2024
@dcharkes
[native_assets_cli] Document exit code behavior (#1762)
e69c74d 
Closes: #33

The behavior is already tested by pkgs/native_assets_builder/test/build_runner/build_runner_build_output_format_test.dart.
@github-project-automation github-project-automation bot moved this from In Progress to Done in Native Assets Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dcharkes dcharkes

Labels
P2 A bug or feature request we're likely to work on package:hooks_runner package:hooks
Projects
Native Assets
Status: Done
Milestone
Native Assets v1.0
Development

Successfully merging a pull request may close this issue.

[native_assets_cli] Document hook exit code behavior dart-lang/native
2 participants
@dcharkes @mkustermann