Repository navigation
[engine_tool] Allow a dot in GN target names - #192423
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the target name validation in the Label class to allow dots, aligning with GN requirements, and adds corresponding unit tests. The review feedback suggests updating the validation error message to be more descriptive of the allowed characters, adhering to the style guide's recommendation that error messages should be useful.
|
|
||
| static FormatException? _checkTarget(String target) { | ||
| if (!_identifier.hasMatch(target)) { | ||
| if (!_targetName.hasMatch(target)) { |
There was a problem hiding this comment.
According to the Flutter Repository Style Guide (under General Philosophy): "Error messages should be useful: Every error message is an opportunity to make someone love our product." Since target names now allow dots and hyphens, the error message "Target name must be a valid identifier." is slightly outdated and could be misleading. Consider updating the error message to be more descriptive of the allowed characters (e.g., "Target name must be a valid GN target name containing only alphanumeric characters, underscores, hyphens, or dots, and starting with a letter or underscore").
References
- Error messages should be useful: Every error message is an opportunity to make someone love our product. (link)
There was a problem hiding this comment.
Done in 7ee04a7. The message now spells out what is accepted:
Target name must start with a letter or underscore and contain only letters, digits, "_", "-", or ".".
I stopped short of calling it a valid GN target name, because this regexp is stricter than GN's: GN also allows + and $, and does not require the first character to be a letter or underscore. Naming GN in the message would promise more than the check actually enforces.
I also updated two doc comments that still described the old rule: the class-level bullet listing valid identifier characters, and the doc on the target field. Those are the ones someone reads when they want to know why compile_gpu_test.dart is accepted but //foo/bar.baz is not.
b-luk
left a comment
b-luk
left a comment
There was a problem hiding this comment.
Thanks for tracking down this issue.
I see no reason why we need a different regex for the package components vs the target name. I think package components may also contain a period, e.g. //flutter/third_party/swiftshader/third_party/llvm-16.0:swiftshader_llvm. Should we just expand the existing regex instead of making a separate one for the target?
|
You're right, and the example is real: DEPS brings in Fixed in 8467016: |
|
Thanks @dzolotov. The change looks good to me. But two issues need addressing:
|
The label parser validated target names with the same pattern it uses for package components, which has no dot. Every Dart test target is named after its test file, so `et` rejected all of them and the group that depends on them, leaving ninja as the only way to build them.
The target regexp now permits a dot, but the FormatException text and the doc comments still said "valid identifier". Spell out the accepted characters in the message, and split the package and target rules in the class documentation.
The separate target pattern was wrong: package components need the dot too. `flutter/third_party/swiftshader/third_party/llvm-16.0` comes in through DEPS and defines `swiftshader_llvm`, so a label pointing at it was rejected before this change and after it as well. The dot goes into `_identifier`, `_targetName` is gone, and both error messages share one sentence describing the rule. The leading letter or underscore still keeps `.`, `..` and `...` out of package names, which matters because `TargetPattern` strips only a trailing `/...` before handing the rest to `Label`.
8467016 to
e4b43bf
Compare
|
Both done. The CLA fix needed a force-push anyway, so I rebased the branch onto today's master (9ead586). One commit had gone out under a second account of mine; all three now carry the address my CLA is signed with, and cla/google is green on the new head, e4b43bf. The diff is identical to what you reviewed. |
|
Thanks, @dzolotov PR process tip: After you've addressed a reviewer's comments, use the "re-request review" button to bring it to the reviewer's attention again. |
|
@gaaclarke for second review (or to pass to someone else) |
gaaclarke
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
Label._checkTargetvalidated target names with_identifier, the same pattern used for package components, which allows no dot.testing/dart/BUILD.gnderives a compile target from each test file name, so all 55 of them -compile_gpu_test.dartand friends - were rejected, along with the:dartgroup that depends on them. Building or running any Dart test throughetwas impossible; ninja was the only way in._identifiergains.here, for both halves of a label. Target names need it forcompile_gpu_test.dart, and package components need it too:DEPSbringsthird_party/swiftshader/third_party/llvm-16.0into the checkout, and itsBUILD.gndefinesswiftshader_llvm. An identifier still has to start with a letter or underscore, so.,..and...remain invalid as a package name component, which is what keeps a label from walking out of the source tree.Fixes #192422
Verified on macOS,
host_debug_unopt_arm64. Both commands from the issue fail on master and succeed with this change:dart test test/label_test.dartinengine_tool: 20 passed.Pre-launch Checklist
///).