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_cli] Consider dynamically injecting verifiers that run after build() / link() methods called user-provided closure #1628

Open
mkustermann opened this issue Oct 3, 2024 · 6 comments

Comments

@mkustermann
Copy link
Member

With the recent refactoring to make the CLI extensible, we removed the verification happening inside the hook, now only the invoker of the hook does the verification.

We could restore this regression by dynamically injecting verifier methods in the extension methods, e.g. output.codeAssets.add() could install a validateCodeAssetBuildOutput verifier into a Set<> that the build implementation will then run.

@dcharkes
Copy link
Collaborator

dcharkes commented Oct 3, 2024

Ah you came up with an idea to achieve this!

I was also thinking that instead of validateCodeAssetBuildOutput in the PR, we could have CodeAsset.validateBuildOutput (static method). Doing that for all asset types makes the validators easy to find.

@dcharkes dcharkes changed the title Consider dynamically injecting verifiers that run after build() / link() methods called user-provided closure [native_assets_cli] Consider dynamically injecting verifiers that run after build() / link() methods called user-provided closure Oct 3, 2024
@dcharkes dcharkes added this to the Native Assets v1.0 milestone Oct 3, 2024
@mkustermann
Copy link
Member Author

CodeAsset.validateBuildOutput (static method). Doing that for all asset types makes the validators easy to find.

I didn't want to do that, because a normal hook/build.dart writer shouldn't have to know about those validation methods, they shouldn't need to call them. So they shouldn't be in scope / available for code completion / ... (without explicitly importing them)

@mkustermann
Copy link
Member Author

mkustermann commented Oct 4, 2024

I still believe that the hook/{build,link}.dart should not do the validation. Conceptually this is the job of the bundling tool, it has to do validation.

Here's a few reasons why I'm convinced of this:

  • Users are not expected to execute these hooks themselves, they should be invoked by a bundling tool which already will do validation and reports it.
  • If users want to write tests for their hooks, then those tests should use a native assets builder infrastructure which supports verification.
  • The bundling tool will do more validation than what a hook/{build,link}.dart hook even can do: per asset type but also across all assets from all packages - the hook itself cannot do that
  • There's a version skew: The bundling tool (Flutter SDK / Dart SDK / ...) may use a different version of the verification logic than the hook writer. That means the bundling tool may do slightly different validation than the self-validation the hook would do
    => That can lead to really weird situations where a hook may throw an exception because it's validation fails but the bundling tool would actually accept the hook output. Or the hook may run validation without errors and the bundling tool has extra validation that fails.
  • We create a situation where validation errors can occur in two places and manifest in two different ways:
    => A hook may throw an exception with a validation error (and possibly print the error to stdout/stderr)
    => The bundling tool may report validation errors in a different way (logging it in to a Logging instance)
    => This isn't a clean user experience because validation errors can be reported in --verbose or log files in two very different ways

So despite my idea of how we could do it, I still think we shouldn't do it: Follow the single-responsibility design principle: Validation is the responsibility of one component, which is the bundling tool.

@dcharkes
Copy link
Collaborator

dcharkes commented Oct 4, 2024

  • Users are not expected to execute these hooks themselves

I believe when the output is erroneous, the easiest way for users to triage the errors is to step through their hook execution with a debugger.

I believe it's better to report errors early instead of letting them propagate.

But let's disagree and commit...

#notPlanned

@dcharkes dcharkes closed this as completed Oct 4, 2024
@dcharkes
Copy link
Collaborator

dcharkes commented Oct 4, 2024

We cannot have validation for temporary asset types (that only live between build and link) in embedders, because embedders don't know about those. So if we want to support validation for those, we should consider implementing this.

@dcharkes dcharkes reopened this Oct 4, 2024
@mkustermann
Copy link
Member Author

We cannot have validation for temporary asset types (that only live between build and link) in embedders, because embedders don't know about those. So if we want to support validation for those, we should consider implementing this.

If we allow arbitrary asset types between build & link then even the current system cannot do any validation between those (i.e. it's unrelated to #1623)

I'd say that's something the asset type support and the linker have to agree / deal with - as the bundling tool really doesn't care about the data that flows from build hooks to link hooks, it only cares about the final assets it has to deal with (and embed in the application bundle).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants