[native_assets_builder] Report dart sources as dependencies of hooks #1780
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
package:native_assets_builder
type-infra
dcharkes
force-pushed
the
dependencies-dart-files
branch
from
51dd61c to
fde2f0a
Compare
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
dcharkes
force-pushed
the
dependencies-dart-files
branch
from
fde2f0a to
90eee5b
Compare
dcharkes
force-pushed
the
dependencies-dart-files
branch
from
90eee5b to
36da1b5
Compare
mkustermann
approved these changes
Dec 6, 2024
| @@ -437,12 +437,12 @@ class NativeAssetsBuildRunner { | |||
| ), | |||
| ); | |||
| if (buildOutput == null) return null; | |||
| hookResult = hookResult.copyAdd(buildOutput); | |||
| hookResult = hookResult.copyAdd(buildOutput, [/*dry run is not cached*/]); | |||
There was a problem hiding this comment.
Didn't you say dry run was finally purged from existence?
Why is this code still here?
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
| @@ -538,7 +538,7 @@ ${e.message} | |||
| [ | |||
| ...result.dependencies, | |||
| // Also depend on the hook source code. | |||
| hookHashesFile.uri, | |||
| hookHashes.file.uri, | |||
There was a problem hiding this comment.
Let's assume we did one build. Now I modify hook/build.dart by adding a few comments.
Next time we run a build, the system sees that hook sources changed and it will re-compile the hook, giving us a new dill file.
Now, since we only changed comments in the hook/build.dart, we will get an identical .dill file.
So the next step is running the hook. Though we can skip running the hook if none of the hook-reported deps changed and the hook.dill didn't change (which it didn't)
=> In this scenario we could avoid running the hook.
Though this code looks like we're going to re-run the hook, because we add the hooks.d file here (where hashes of sources changed, we we added some comments).
(One could argue this redundant run is ok because it's very uncommon, but from conceptual point we should only re-run things when needed)
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
|
Thanks @mkustermann! I'll address the two remaining comments in separate PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #1770
This PR adds the Dart sources to the dependencies in
HookResult.It also fixes the dep-file parsing w.r.t. to escapes.
Implementation details
We're passing around
HookOutputs which is the deserializedoutput.json. We could add the Dart sources as dependencies to it after deserializing, but then the correspondence to the json is lost. So instead I've added an extra return value to the places where we passHookOutputaround.