David Turner would like Owners Override to review this change.
[gn] Roll prebuilt GN manually
This rolls the Fuchsia GN prebuilt to a new version
git_revision:7ecf26c4148fd141741775347af89aa9c00a80bc
which changed the formatter to impose a stricter 80 line
width limit [1], and thus requires manually re-running
`bazel2gn` to fix BUILD.bazel / BUILD.gn mismatches, and
then `fx format-code --all -- "*.gn" "*.gni"`
[1]: https://gn-review.googlesource.com/c/gn/+/21820
This was created by doing the following:
1) Updating manifest/toolchain to update the GN
git_revision number, then calling update-lockfiles.sh
in this directory.
2) Using 'jiri update -local-manifest-project=fuchsia'
to get the latest GN prebuilt in the checkout
3) Reformat all GN files in fuchsia.git with
`git ls-files | grep -E -e '\.gni?$' | xargs prebuilt/third_party/gn/linux-x64/gn format`
4) Manually update //tools/create/templates/driver-default/BUILD.gn.tmpl-cpp
to fix its formatting (as `gn format` doesn't like the substitution
expressions in that file) and
//tools/create/goldens/my-driver-cpp/BUILD.gn to match its new version.
NO_IFTTT=Too many violations in existing files for this CL
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Fuchsia-Auto-Submit | +1 |
Adding owners-override to get progress on this CL, I'll rebase it on top of recent changes of course.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adding owners-override to get progress on this CL, I'll rebase it on top of recent changes of course.
I don't think an owners-override vote will persist through the conflict resolution, so you'll have to do that first.
What's the plan to follow up on the IFTTT failures?
James RobinsonAdding owners-override to get progress on this CL, I'll rebase it on top of recent changes of course.
I don't think an owners-override vote will persist through the conflict resolution, so you'll have to do that first.
What's the plan to follow up on the IFTTT failures?
James RobinsonAdding owners-override to get progress on this CL, I'll rebase it on top of recent changes of course.
David TurnerI don't think an owners-override vote will persist through the conflict resolution, so you'll have to do that first.
What's the plan to follow up on the IFTTT failures?
Oh, good to know, thank you. Will do.
So regarding the IFTTT failures, I took a deep look at those revealed by the initial patchset and found that most are harmless (i.e. a comment within a LINT block is reformatted). However, there are two cases where the reformat did
mess the LINT.IfChange() line. I'm going to upload a new patch with a recent rebase without the NO_IFTTT line to surface all issues and have a new look to fix all these properly first.
Then another patchset with NO_IFTTT and the fixes applied and explained in the commit message.
# LINT.ThenChange(BUILD.bazel:schema_in_idk,
# //build/sdk/sdk_common/__init__.py:idk_atom_types)David Turnerdoes this work? or does it need to be on the same line?
Oliver NewmanI don't see shac complaining about it, So I assume it works. Otherwise we're going to have a major problem preventing us from rolling this new GN version.
Clayton WilkinsonIFTTT is enforced by Aye-Aye rather than shac, but looking at the code it does appear to support multiple lines :) https://screenshot.googleplex.com/eJTK58ukK8gQLeF
David TurnerGreat - maybe as a follow-up it would be good to confirm it, and/or suggest updating the documentation for IFTTT that it supports this multi-line format
Good idea, I have uploaded https://fuchsia-review.googlesource.com/c/fuchsia/+/1620114 to update our fuchsia.dev documentation.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| SLSA-Policy-Verified | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| SLSA-Policy-Verified | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Fuchsia-Auto-Submit | +1 |
I have reviewed all violations manually, and adjusted the one that was triggerred by the reformatting (see commit message). @jamesr, please tell me what you think about this CL.
James RobinsonAdding owners-override to get progress on this CL, I'll rebase it on top of recent changes of course.
David TurnerI don't think an owners-override vote will persist through the conflict resolution, so you'll have to do that first.
What's the plan to follow up on the IFTTT failures?
David TurnerOh, good to know, thank you. Will do.
So regarding the IFTTT failures, I took a deep look at those revealed by the initial patchset and found that most are harmless (i.e. a comment within a LINT block is reformatted). However, there are two cases where the reformat did
mess the LINT.IfChange() line. I'm going to upload a new patch with a recent rebase without the NO_IFTTT line to surface all issues and have a new look to fix all these properly first.Then another patchset with NO_IFTTT and the fixes applied and explained in the commit message.
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | -1 |
| Fuchsia-Auto-Submit | +1 |
I am going to abandon this CL because it's far too large and there are too many conflicts on each rebase, with the non-zero change of introducing regression because nobody has time to review all files again and again.
New strategy will be the following:
- The new GN does two things which are quite different: 1) it restricts
the wrapping width to 80 columns, 2) it changes how some list expressions
are indented and other very minor styling issues.
Most of the changes in this CL deal with 1) due to long comments, and it is possible to land the fixed version in the tree today, without changing our
GN version. These can be generated with simply calling the `format`
command of the new GN, followed by the `format` version of the current one
(which will restore the stylistic differences, but not the rewrapped
comments).
Hence I'll update smaller CLs for different sub-systems to perform this
comment reformatting only.
For 2), it is necessary to land the corresponding change with the new GN
binary in the tree, but fortunately, these cases are far more limited and
will correspond to a new version of this CL with far less changes to review.
Sorry for the noise, and thanks for your time.
# luci.turquoise.global.try:bringup.vim3-debug,core.vim3-debug,core.vim3-vg-debugnit: this is bad, will fix/
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |