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

[native_assets_builder] If running hook/build.dart of a package fails, then we should not continue building dependent packages #1630

Closed
mkustermann opened this issue Oct 4, 2024 · 1 comment · Fixed by #1768

Comments

@mkustermann
Copy link
Member

It seems currently the build runner will make a plan (order of packages) and then build all of them. See here

  Future<HookResult> _run({}) async {
    ...
    final (buildPlan, packageGraph, planSuccess) = await _makePlan(...);
    ...
    for (final package in buildPlan) {
    ...
    final (hookOutput, packageSuccess) = await _runHookForPackageCached(...);
    final validateResult = validateNoDuplicateDylibs(hookResult.assets);
    if (validateResult.isNotEmpty) {
      for (final error in validateResult) {
        logger.severe(error);
      }
      hookResult = hookResult.copyAdd(HookOutputImpl(), false);
    }

    return hookResult;
  }

I don't see any path where it fails early, which seems problematic: If package:foo depends on package:bar, both of them have build hooks and foos hook depends on outputs from bars hook, then if bar fails to build, we shouldn't even attempt to build foo.

Conservatively it could fail after the first package that failed to build or verify correctly.

Slightly better would be to have a graph (or more precisely a DAG) and build everything whose dependencies have transitively been successfully built, but never attempt to build a package that has any transitive dependencies with build failures.

@dcharkes dcharkes changed the title If running hook/build.dart of a package fails, then we should not continue building dependent packages [native_assets_builder] If running hook/build.dart of a package fails, then we should not continue building dependent packages Nov 28, 2024
@dcharkes dcharkes added this to the Native Assets v1.0 milestone Nov 28, 2024
@dcharkes
Copy link
Collaborator

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

I'll fix this, or if it's already fixed, add a regression test.

Conservatively it could fail after the first package that failed to build or verify correctly.

This is what ninja does. It does not try to build all reachable targets. It stops building new targets after it encounters the first failure. (Ninja does finish building all targets that are already being built concurrently with -j1000.)

Slightly better would be to have a graph (or more precisely a DAG) and build everything whose dependencies have transitively been successfully built, but never attempt to build a package that has any transitive dependencies with build failures.

That increases the latency. And requires users to ctrl+c the build if they already know how to fix the issue. So, maybe it's better if we stick to early failure like ninja.

@dcharkes dcharkes self-assigned this Nov 28, 2024
@dcharkes dcharkes moved this to In Progress in Native Assets Nov 28, 2024
@auto-submit auto-submit bot closed this as completed in a97e338 Dec 2, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Native Assets Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants