-
Notifications
You must be signed in to change notification settings - Fork 6k
Enabling pre-push checks on Windows #36123
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| final List<WorkerJob> jobs = patches.map<WorkerJob>((String patch) { | ||
| return WorkerJob( | ||
| <String>['patch', '-p0'], | ||
| <String>['git', 'apply', '--ignore-space-change'], |
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.
From my understanding, --ignore-space-change might ignore real problems. Should we only add --ignore-space-change if on Platform.isWindows?
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 for reviewing. Can you share what problems this might ignore? I'm asking because if those problems are present on Windows as well, then making it conditional does not solve the problem, only limits the damage it can done. I'd much rather solve it proper, if possible.
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.
From my understanding this fixes formatting issues, and --ignore-space-change is to skip line ending changes on Windows. If formatting replaced tabs with spaces, would that get ignored here?
I'm asking because if those problems are present on Windows as well, then making it conditional does not solve the problem, only limits the damage it can done. I'd much rather solve it proper, if possible.
I completely agree 👍
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 wonder if on Windows only we could enable --ignore-cr-at-eol instead if that's currently causing an issue with the formatter? I assume that on push, we're normalising line endings to unix line endings?
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.
The issue this switch is trying to solve comes from clang-tidy on Windows. No matter what line endings you feed into it, the output is always CRLF EOF. So I use --ignore-cr-at-eol at patch creation in order to skip patching line endings to CRLF for the whole file.
As for the patch application side, since --ignore-cr-at-eol is a git diff-only switch, I needed to find some other solution to avoid line ending differences tripping the context matching in git apply. If I remove this switch now, git apply says the patch does not apply (obviously, since the context lines in the patch differ in EOL if files are checked out with LF line endings).
I reviewed the --ignore-space-change switch documentation and I still can't see where this can cause a problem. The switch only controls how the application context is being checked, it does not change the patch itself. So if clang-tidy generates a properly formatted file, git diff will create the proper patch. Thus git apply (I think) cannot "misapply" the patch if the file did not change since running format.
Alternatively I can patch the output of clang-tidy EOL, but that could become complicated as it's not guaranteed that files on Windows are checked out with CRLF. You can easily check out files with the proper LF EOL, so that needs to be detected before patching the clang-tidy output.
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.
Ah I see! Makes sense. Not sure why I misread that as git diff when it was clearly the patch command, probably mixing it up with the diff bits later. Thanks for clarifying.
|
I might be missing something, but should this PR update the githooks path? engine/tools/githooks/setup.py Lines 30 to 32 in dbb5284
|
Since that was the cause of the big miscommunication contributing to the rollback (namely that people need to run Whichever way this goes I will be able to make the changes and follow up with the necessary tests in the second half of December. |
|
When I test this locally, I get a pre-push warning even after I run I'm a bit confused. Does this change intend to keep a Windows pre-push hook to kick off formatting? If so, it looks like the current Windows pre-push hook is incorrect (it outputs a warning instead of kicking off formatting). Or does this change intend to have a single pre-push hook for all platforms? If so, it seems |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env python3 | |||
| #!/usr/bin/env vpython3 | |||
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.
Avoids issue with Windows setting up alias for python3 to the Microsoft Store by default. This will also make sure that every install uses the depot_tools python version.
|
@loic-sharma Haha you're right, I confused myself along the way and this was missing. I added it now. |
|
@cbracken @yaakovschectman would you mind testing that this works as expected on your Windows machine?
|
|
It is catching the formatting on my end |
cbracken
left a comment
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'll give a test on my Windows machine. Have we re-tested everything looks good for non-Windows too?
|
Added tests for One way forward could be to refactor |
Yup I tested this on my mac, Windows, and on WSL |
cbracken
left a comment
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.
|
Hi, if the tests are sufficient on this PR, can we remove the "needs tests" label? Also is there something else I need to do to get this merged? Thanks! |
|
@mtolmacs I rebased this on the latest Incorrect formatting...Would you be able to take a look at that? Those formatting recommendations seem incorrect. Attempting to format those same documents within Visual Studio does not produce any changes. Also, I removed the test label :) |
|
This is indeed a breaking bug, the C++ formatting patch clearly cuts off the end of the file before diffing. Let me investigate this. In the meantime, this should not be merged. Thanks for catching it! |
|
For reference, this is the dartlang issue associated with this bug: dart-lang/sdk#50904 |
|
@mtolmacs Is this PR still on your radar? Should we find a workaround for the Dart bug while we wait for it to be fixed? |
|
Hi @Hixie yes. It's functionally complete, the only thing needs fixing is the output issue this is blocked on currently. I don't have any other ideas how this can be worked around outside the Dart codebase economically, but I'm open to ideas. |
|
/cc @brianquinlan in case he can think of any workarounds for dart-lang/sdk#50904 (desktop triage) |
in the Windows ci/format.bat instead of //third_party/dart/tools/sdks/dart-sdk
|
(sorry for the accidental self-rebase pushed, I was testing git push and was careless - reverted the commits in 664990c) I've made the modification in Question regarding the new |
|
The engine tests are (mostly) centrally-managed in run_tests.py which is wired into CI in the build config files such as this one. I think we're probably fine to land this as-is for the moment and land a followup patch that wires the new test into the python script. |
|
@mtolmacs FYI, I added your tests to the CI in EDIT: The Java format check failed if Java isn't on your path. I tried switching to the Java executable in the |
|
I fixed the package installation in the test. Should be the last issue. Let me know if you need anything else to get this over the finish line. |
zanderso
left a comment
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.
A few style nits. Otherwise lgtm.
Co-authored-by: Zachary Anderson <[email protected]>
Co-authored-by: Zachary Anderson <[email protected]>
Co-authored-by: Zachary Anderson <[email protected]>
|
@mtolmacs, thank you for sticking through this and landing this! Wonderful work! 🥳 |
…129306) flutter/engine@090fae8...08aaa88 2023-06-21 [email protected] Enabling pre-push checks on Windows (flutter/engine#36123) 2023-06-21 [email protected] Document the use of contexts on engine_v2 tests. (flutter/engine#43013) 2023-06-21 [email protected] Generate treemap. (flutter/engine#43029) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Re-submit the changes to enable windows pre-push checks.
This patch changes how
ci/bin/format.dartgenerate diffs fromdiffandpatchcommands togit diffandgit applyin order to have a common method for these operations on all platforms. Windows installations don't have diff and patch commands available by default and many implementations which provide such commands work differently than the UN*X tools. Git however works consistently across all platforms.Additionally, this patch also changes the python executable in some of the pre-push components affected by this to
vpython3to continue the effort started at flutter/flutter#108474 and I also removed the--no-sound-null-safetyparameter in the ci/format.sh, ci/format.bat filesNOTE: Since the original patch caused some issues, I suggest that this should be tested more carefully before it is merged.
Issues fixed by this PR
git pushis broken on Windows flutter#108122flutter/tests repo impact
None.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.