-
Notifications
You must be signed in to change notification settings - Fork 216
Add "triggers" to quickly decide when to not run builders. #4084
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
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
466580e
to
5deeaae
Compare
b575b09
to
0551fa2
Compare
b7baf18
to
48f8008
Compare
48f8008
to
1329b6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an awful lot of manual config that's bound to go wrong at some point.
We can't do something similar without manual config?
|
||
@override | ||
bool triggersOnPrimaryInput(String source) { | ||
return source.contains('package:$import'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically an import string can be split into multiple lines, e.g.
import 'd' /* hello */ 'a'
//there
'r'
// I'm
't'
// a
':'
// perfectly
'i'
// valid
'o'
// import
''
// !
;
(and I have seen than --- not as absurdly as I did above but as I recall to make it fit on lines or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hesitant to do full parsing simply because there is no convenient place to dedupe the parsing work; but I do think it's better and the way to go in the end.
Filed #4128 to figure out a way to save parsing multiple times, and changed the code to do parsing so annotation + import checks are both (hopefully) now correct.
|
||
@override | ||
bool triggersOnPrimaryInput(String source) { | ||
return source.contains('@$annotation'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe @ myAnnotation
is valid too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by switch to parsing.
Also, I realized that annotations should be checked for in all compilation units, it's not like imports where they can only be in the main library. So, I added finding and loading all the parts for the check, and test coverage for that.
for (final entry in triggersByBuilder.entries) { | ||
final builderName = entry.key; | ||
|
||
if (!builderName.contains(_builderNamePattern)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this be found below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the checks below are just on the triggers, not the builder names.
/// message. | ||
/// | ||
/// The only supported trigger is [ImportBuildTrigger]. | ||
static (BuildTrigger?, String?) tryParse(String trigger) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be private?
If it has to be public I think I'd prefer a "proper return type" rather than a record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; made private.
f58f003
to
9e3fff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, please take another look.
Re: config-based: the fact that it's config based rather than code means that the builder package, the user package and any additional package in the build can all contribute triggers.
Usually, only the builder package will want to define triggers, which would work in code; but then if you want to add a new way to trigger, e.g. to re-export an annotation, you're stuck.
My expectation is that for most builders, just a trigger defined with the builder works 95% of the time, and then a small number of advanced users defining new ways to run the builder can do what they want for the remaining 5%. One important case is one builder that triggers another, as freezed does with json_serializable ... that should work.
There's a suggestion to add a lint to point out cases where a trigger should be defined #4086
for (final entry in triggersByBuilder.entries) { | ||
final builderName = entry.key; | ||
|
||
if (!builderName.contains(_builderNamePattern)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the checks below are just on the triggers, not the builder names.
/// message. | ||
/// | ||
/// The only supported trigger is [ImportBuildTrigger]. | ||
static (BuildTrigger?, String?) tryParse(String trigger) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; made private.
|
||
@override | ||
bool triggersOnPrimaryInput(String source) { | ||
return source.contains('package:$import'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hesitant to do full parsing simply because there is no convenient place to dedupe the parsing work; but I do think it's better and the way to go in the end.
Filed #4128 to figure out a way to save parsing multiple times, and changed the code to do parsing so annotation + import checks are both (hopefully) now correct.
|
||
@override | ||
bool triggersOnPrimaryInput(String source) { | ||
return source.contains('@$annotation'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by switch to parsing.
Also, I realized that annotations should be checked for in all compilation units, it's not like imports where they can only be in the main library. So, I added finding and loading all the parts for the check, and test coverage for that.
name = '$name.${metadata.constructorName}'; | ||
} | ||
if (annotation == name) return true; | ||
final periodIndex = name.indexOf('.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we end up comparing to metadata.constructorName
or could name
(before line 216) have dots in it where we want to remove the first part? Could there be a comment explaining? (and either compare directly to metadata.constructorName
or make sure we don't depending on what we're actually trying to achieve)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added comment + a bit of test coverage on the awkward case. (Import prefixes and class names can't actually be distinguished).
For #4083
Before/after incremental builds with a locally-configured annotation trigger for built_value, this with the
--mostly-no-codegen
option so most files should not run the builder.before
after
999 have changed from no-op to skipped: previously the builder would run and analyze all files, now build_runner has noticed that only a primary input change can affect triggering for 999 files and so they are skipped.