Enabling pre-push checks on Windows

[mtolmacs]

Re-submit the changes to enable windows pre-push checks.

This patch changes how ci/bin/format.dart generate diffs from diff and patch commands to git diff and git apply in 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 vpython3 to continue the effort started at flutter/flutter#108474 and I also removed the --no-sound-null-safety parameter in the ci/format.sh, ci/format.bat files

NOTE: 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

• flutter/flutter#108122
• flutter/flutter#107920
• flutter/flutter#86506
• flutter/flutter#106615

flutter/tests repo impact

None.

Pre-launch Checklist

• [X] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [X] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [X] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• [X] I listed at least one issue that this PR fixes in the description above.
• [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• [X] I updated/added relevant documentation (doc comments with ///).
• [X] I signed the CLA.
• [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard[bot]]

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.

[loic-sharma]

I might be missing something, but should this PR update the githooks path?

https://github.com/flutter/engine/blob/dbb5284f2c8d87473d60baa21bba0b7ab88c8281/tools/githooks/setup.py#L30-L32

[mtolmacs]

I might be missing something, but should this PR update the githooks path?

https://github.com/flutter/engine/blob/dbb5284f2c8d87473d60baa21bba0b7ab88c8281/tools/githooks/setup.py#L30-L32

Since that was the cause of the big miscommunication contributing to the rollback (namely that people need to run gclient sync in order for the hook setup to update), I though this way around let's not change that. I can bring back the unified githook path and leave a git.bat in the windows folder which still does the job but warns the user to update if you think it's the right way to go.

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.

[loic-sharma]

When I test this locally, I get a pre-push warning even after I run gclient sync -D:

> gh co 36123
> git push -u origin HEAD
Warning: Pre-push checks are not enabled!
Note: Please run 'gclient sync -D' to enable pre-push checks on Windows
...
> gclient sync -D
Updating depot_tools...
...
Running hooks: 100% (10/10), done.
> git push origin HEAD
Warning: Pre-push checks are not enabled!
Note: Please run 'gclient sync -D' to enable pre-push checks on Windows
Everything up-to-date

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 setup.py should be updated. Please let me know if I misunderstood something.

[mtolmacs]

@loic-sharma Haha you're right, I confused myself along the way and this was missing. I added it now.

[loic-sharma]

@cbracken @yaakovschectman would you mind testing that this works as expected on your Windows machine?

1. Modify a file locally such that its formatting is incorrect
2. Checkout this branch:

> git checkout -b mtolmacs-reenable-win-prepush main
> git pull [email protected]:mtolmacs/flutter-engine.git reenable-win-prepush

3. Check you get a warning if the pre-push hook path hasn't been updated yet:

> git push -u origin HEAD
Warning: Pre-push checks are not enabled!
Note: Please run 'gclient sync -D' to enable pre-push checks on Windows
...

4. Update your pre-push hook

> gclient sync -D
Updating depot_tools...
...
Running hooks: 100% (10/10), done.

5. Verify it catches formatting issues:

> git push origin HEAD
The clang-tidy check is disabled. To enable set the environment variable PRE_PUSH_CLANG_TIDY to any value.
Starting formatting checks.
Check "Formatting check" failed.
...
formatting checks finished in 0:00:02.547010
pre-push checks finished in 0:00:02.555945
error: failed to push some refs to 'github.com:loic-sharma/flutter-engine.git'

[yaakovschectman]

It is catching the formatting on my end

[mtolmacs]

Added tests for ci/bin/format.dart in 460ad05. Unfortunately the tests need to run in a serialized manner due to the fact that the formatting process cannot run in parallel. I solved it by using the 'sync' versions of dart:io commands, which is not ideal, but gets the job done.

One way forward could be to refactor ci/bin/format.dart to make it testing friendly, which might be out of scope for this PR. Let me know if you see a better approach and I'm happy to rewrite accordingly.

[loic-sharma]

Have we re-tested everything looks good for non-Windows too?

Yup I tested this on my mac, Windows, and on WSL

[mtolmacs]

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!

[loic-sharma]

@mtolmacs I rebased this on the latest main branch and got some incorrect formatting errors:

Incorrect formatting...

PS C:\Code\f\engine\src\flutter> git push
The clang-tidy check is disabled. To enable set the environment variable PRE_PUSH_CLANG_TIDY to any value.
Starting formatting checks.
Check "Formatting check" failed.
command: C:\Code\f\engine\src\flutter\ci\format.bat
working directory: C:\Code\f\engine\src\flutter
exit code: 1
stdout:
To fix, run:

git apply <<DONE
diff --git a/impeller/entity/contents/color_source_contents.cc b/impeller/entity/contents/color_source_contents.cc
index c95507ea4f..0000000000 100644
--- a/impeller/entity/contents/color_source_contents.cc
+++ b/impeller/entity/contents/color_source_contents.cc
@@ -38,17 +38,4 @@ const Matrix& ColorSourceContents::GetInverseMatrix() const {
 }

 std::optional<Rect> ColorSourceContents::GetCoverage(
-    const Entity& entity) const {
-  return geometry_->GetCoverage(entity.GetTransformation());
-};
-
-bool ColorSourceContents::ShouldRender(
-    const Entity& entity,
-    const std::optional<Rect>& stencil_coverage) const {
-  if (!stencil_coverage.has_value()) {
-    return false;
-  }
-  return Contents::ShouldRender(entity, stencil_coverage);
-}
-
-}  // namespace impeller
+    const Entity& entity)
\ No newline at end of file
diff --git a/impeller/entity/contents/color_source_contents.h b/impeller/entity/contents/color_source_contents.h
index 4396c38395..0000000000 100644
--- a/impeller/entity/contents/color_source_contents.h
+++ b/impeller/entity/contents/color_source_contents.h
@@ -39,11 +39,4 @@ class ColorSourceContents : public Contents {
   Scalar GetAlpha() const;

  private:
-  std::shared_ptr<Geometry> geometry_;
-  Matrix inverse_matrix_;
-  Scalar alpha_ = 1.0;
-
-  FML_DISALLOW_COPY_AND_ASSIGN(ColorSourceContents);
-};
-
-}  // namespace impeller
+  std::share
\ No newline at end of file
diff --git a/impeller/playground/compute_playground_test.h b/impeller/playground/compute_playground_test.h
index 06c480b5be..0000000000 100644
--- a/impeller/playground/compute_playground_test.h
+++ b/impeller/playground/compute_playground_test.h
@@ -39,13 +39,4 @@ class ComputePlaygroundTest
  private:
   fml::TimeDelta start_time_;

-  FML_DISALLOW_COPY_AND_ASSIGN(ComputePlaygroundTest);
-};
-
-#define INSTANTIATE_COMPUTE_SUITE(playground)                              \
-  INSTANTIATE_TEST_SUITE_P(                                                \
-      Compute, playground, ::testing::Values(PlaygroundBackend::kMetal),   \
-      [](const ::testing::TestParamInfo<ComputePlaygroundTest::ParamType>& \
-             info) { return PlaygroundBackendToString(info.param); });
-
-}  // namespace impeller
+  FML_DISALLOW_COPY_AND_ASSIGN(ComputePlaygroundTes
\ No newline at end of file
diff --git a/impeller/scene/importer/conversions.h b/impeller/scene/importer/conversions.h
index cb11d3c42f..0000000000 100644
--- a/impeller/scene/importer/conversions.h
+++ b/impeller/scene/importer/conversions.h
@@ -41,14 +41,4 @@ std::unique_ptr<fb::Matrix> ToFBMatrixUniquePtr(const Matrix& m);

 fb::Vec2 ToFBVec2(const Vector2 v);

-fb::Vec3 ToFBVec3(const Vector3 v);
-
-fb::Vec4 ToFBVec4(const Vector4 v);
-
-fb::Color ToFBColor(const Color c);
-
-std::unique_ptr<fb::Color> ToFBColor(const std::vector<double>& c);
-
-}  // namespace importer
-}  // namespace scene
-}  // namespace impeller
+fb::Vec3 ToFBVec3(const Ve
\ No newline at end of file
diff --git a/shell/platform/windows/testing/flutter_windows_engine_builder.h b/shell/platform/windows/testing/flutter_windows_engine_builder.h
index cc41f03fee..0000000000 100644
--- a/shell/platform/windows/testing/flutter_windows_engine_builder.h
+++ b/shell/platform/windows/testing/flutter_windows_engine_builder.h
@@ -27,17 +27,4 @@ class FlutterWindowsEngineBuilder {

   // Prevent copying.
   FlutterWindowsEngineBuilder(FlutterWindowsEngineBuilder const&) = delete;
-  FlutterWindowsEngineBuilder& operator=(FlutterWindowsEngineBuilder const&) =
-      delete;
-
- private:
-  WindowsTestContext& context_;
-  FlutterDesktopEngineProperties properties_ = {};
-  std::string dart_entrypoint_;
-  std::vector<std::string> dart_entrypoint_arguments_;
-};
-
-}  // namespace testing
-}  // namespace flutter
-
-#endif  // FLUTTER_SHELL_PLATFORM_WINDOWS_TESTING_FLUTTER_WINDOWS_ENGINE_BUILDER_H_
+  FlutterWindowsEngineBuilde
\ No newline at end of file
diff --git a/third_party/accessibility/ax/platform/ax_platform_tree_manager.h b/third_party/accessibility/ax/platform/ax_platform_tree_manager.h
index e9ee973ce2..0000000000 100644
--- a/third_party/accessibility/ax/platform/ax_platform_tree_manager.h
+++ b/third_party/accessibility/ax/platform/ax_platform_tree_manager.h
@@ -27,13 +27,4 @@ class AX_EXPORT AXPlatformTreeManager : public AXTreeManager {
       const AXNode::AXID node_id) const = 0;

   // Returns an AXPlatformNode that corresponds to the given |node|.
-  virtual AXPlatformNode* GetPlatformNodeFromTree(const AXNode& node) const = 0;
-
-  // Returns an AXPlatformNodeDelegate that corresponds to a root node
-  // of the accessibility tree.
-  virtual AXPlatformNodeDelegate* RootDelegate() const = 0;
-};
-
-}  // namespace ui
-
-#endif  // UI_ACCESSIBILITY_PLATFORM_AX_PLATFORM_TREE_MANAGER_H_
+  virtual AXPlat
\ No newline at end of file
DONE

To fix, run:

git apply <<DONE
diff --git a/impeller/scene/BUILD.gn b/impeller/scene/BUILD.gn
index 69c024f9e4..0000000000 100644
--- a/impeller/scene/BUILD.gn
+++ b/impeller/scene/BUILD.gn
@@ -43,15 +43,4 @@ impeller_component("scene") {
   deps = [ "//flutter/fml" ]
 }

-impeller_component("scene_unittests") {
-  testonly = true
-
-  sources = [ "scene_unittests.cc" ]
-
-  deps = [
-    ":scene",
-    "../fixtures",
-    "../playground:playground_test",
-    "//flutter/testing:testing_lib",
-  ]
-}
+impeller_component("sc
\ No newline at end of file
diff --git a/shell/platform/darwin/common/BUILD.gn b/shell/platform/darwin/common/BUILD.gn
index a3210bfed4..0000000000 100644
--- a/shell/platform/darwin/common/BUILD.gn
+++ b/shell/platform/darwin/common/BUILD.gn
@@ -35,20 +35,4 @@ source_set("framework_shared") {

   sources = [
     "framework/Source/FlutterChannels.mm",
-    "framework/Source/FlutterCodecs.mm",
-    "framework/Source/FlutterStandardCodec.mm",
-    "framework/Source/FlutterStandardCodecHelper.cc",
-    "framework/Source/FlutterStandardCodec_Internal.h",
-  ]
-
-  public = framework_shared_headers
-
-  defines = [ "FLUTTER_FRAMEWORK" ]
-
-  public_configs = [
-    "//flutter:config",
-    ":framework_relative_headers",
-  ]
-
-  deps = [ "//flutter/fml" ]
-}
+    "framework/S
\ No newline at end of file
DONE


stderr:
Performing C++/ObjC/Shader format check
Checking C++/ObjC/Shader formatting...
ERROR: Found 6 C++/ObjC/Shader files which were formatted incorrectly.
Found C++/ObjC/Shader format problems.
Performing GN format check
Checking GN formatting...
ERROR: Formatter command 'C:\Code\f\engine\src\flutter\third_party\gn\gn.exe format --stdin' failed with exit code 1. Command output follows:

ERROR at :28:14: Expecting assignment or function call.
  testonly = true
             ^---

ERROR: Found 2 GN files which were formatted incorrectly.
Found GN format problems.
Performing Java format check
Checking Java formatting...
ERROR: Cannot run Java, skipping Java file formatting!
Performing Python format check
Checking Python formatting...
All python files formatted correctly.
Performing Trailing whitespace format check
Checking for trailing whitespace on 336 source files...
No trailing whitespace found.

formatting checks finished in 0:00:04.654889
pre-push checks finished in 0:00:04.664406
error: failed to push some refs to 'github.com:loic-sharma/flutter-engine.git'

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 :)

[mtolmacs]

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!

[mtolmacs]

For reference, this is the dartlang issue associated with this bug: https://github.com/dart-lang/sdk/issues/50904

[Hixie]

@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?

[mtolmacs]

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.

[cbracken]

/cc @brianquinlan in case he can think of any workarounds for https://github.com/dart-lang/sdk/issues/50904 (desktop triage)

[loic-sharma]

@mtolmacs if you rebase you should be unblocked now! The Dart SDK fix was rolled into the engine yesterday by https://github.com/flutter/engine/commit/ff7adb82f6169c7df424762a8440ff7ac71c9bc3.

[mtolmacs]

Yes, thanks, I'll check it in the next couple of days.

[cbracken]

Added a teensy cleanup to make the linter happy (and rebased).

[cbracken]

I've marked this as ready for review since the underlying blocker is now resolved. @mtolmacs can take one last pass to be sure he's good to go and reply re @loic-sharma's pubspec question above, and then let's land this thing 🚀.

Re-lgtm from me too! (And thanks for your patience!)

[mtolmacs]

I checked it with the latest Dart build from the tree (--no-prebuilt-dart-sdk) and removed the deprecated hack + tested it on Windows (I still need to test it on Mac and Linux locally), also adressing the pubspec.yaml question.

However the scripts hard-code the pre-built Dart SDK from the third-party directory, so we need to wait until the prebuild Dart SDK is updated to 3.1.0-189.0.dev. I'm now sure how I can monitor that, so I will periodically check the tree and see if it's updated. Until then I'll keep testing it.

EDIT: Tested it on Ubuntu Linux Jammy and macOS Ventura, all reporting and fixing seems to be working as intended.

[loic-sharma]

However the scripts hard-code the pre-built Dart SDK from the third-party directory, so we need to wait until the prebuild Dart SDK is updated to 3.1.0-189.0.dev. I'm now sure how I can monitor that, so I will periodically check the tree and see if it's updated. Until then I'll keep testing it.

The Dart SDK in //third_party/dart should've been updated by https://github.com/flutter/engine/commit/ff7adb82f6169c7df424762a8440ff7ac71c9bc3. You should be able to get it locally using gclient sync. Please let me know if I'm missing something though!

[mtolmacs]

However the scripts hard-code the pre-built Dart SDK from the third-party directory, so we need to wait until the prebuild Dart SDK is updated to 3.1.0-189.0.dev. I'm now sure how I can monitor that, so I will periodically check the tree and see if it's updated. Until then I'll keep testing it.

The Dart SDK in //third_party/dart should've been updated by ff7adb8. You should be able to get it locally using gclient sync. Please let me know if I'm missing something though!

I just ran gclient sync -D on main (Windows 11) and here's what //third_party/dart/tools/sdks/dart-sdk/bin/dart.exe --version reports:

Dart SDK version: 3.1.0-155.0.dev (dev) (Fri May 26 19:56:47 2023 -0700) on "windows_x64"

What am I missing?

[cbracken]

@mtolmacs you can build against the non-prebuilt Dart SDK by passing the --no-prebuilt-dart-sdk option to flutter\tools\gn. Sorry about the confusion.

@zanderso how frequently does the prebuilt Dart SDK roll in the engine? I can't remember if it's a CIPD package or something else. I had assumed it was updating roughly on the same frequency as the Dart SDK rolled into the engine.

[loic-sharma]

Ah interesting, it looks like we have two prebuilt Dart SDKs:

1. //third_party/dart/tools/sdks/dart-sdk/bin/dart.exe
2. //flutter/prebuilts/windows-{x64,arm64}/dart-sdk/bin/dart.exe

On my machine, //flutter/prebuilts/windows-x64/dart-sdk/bin/dart.exe reports:

Dart SDK version: 3.1.0-208.0.dev (dev) (Tue Jun 13 20:07:42 2023 -0700) on "windows_x64"

The build uses //flutter/prebuilts/ for its pre-built Dart SDK, it looks like we should update the Windows and non-Windows format scripts to reflect that.

(If we do need to update //third_party/dart/tools/sdks/dart-sdk/bin/dart.exe, it seems these are the steps on the Dart repo).

[mtolmacs]

(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 ci/format.bat (and that file only, so not touching format.sh) to use the //flutter/prebuilts/{platform}/dart-sdk version of Dart. Tested it on Windows, so we can proceed if everyone approves.

Question regarding the new ci/test/format_test.dart file: Is there anything else needed here (for example hook this up with automated CI)? Currently it needs to be ran manually at the discretion of the developer.

[cbracken]

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.

[loic-sharma]

@mtolmacs FYI, I added your tests to the CI in 150a5a7 (#36123). Let me know if you have feedback.

EDIT: The Java format check failed if Java isn't on your path. I tried switching to the Java executable in the //third_party/java/open_jdk dependency, but that dependency isn't downloaded on all platforms. I've disabled the Java format test for now, we can add it later: https://github.com/flutter/flutter/issues/129221

[mtolmacs]

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.

[loic-sharma]

@mtolmacs, thank you for sticking through this and landing this! Wonderful work! 🥳