Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@mtolmacs
Copy link
Contributor

@mtolmacs mtolmacs commented Sep 13, 2022

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/tests repo impact

None.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • 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.
  • I updated/added relevant documentation (doc comments with ///).
  • 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
Copy link

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'],
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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 👍

Copy link
Member

@cbracken cbracken Dec 12, 2022

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?

Copy link
Contributor Author

@mtolmacs mtolmacs Dec 13, 2022

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.

Copy link
Member

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.

@loic-sharma
Copy link
Member

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

if IsWindows():
git = 'git.bat'
githooks = os.path.join(githooks, 'windows')

@mtolmacs
Copy link
Contributor Author

mtolmacs commented Dec 8, 2022

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

if IsWindows():
git = 'git.bat'
githooks = os.path.join(githooks, 'windows')

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
Copy link
Member

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.

@@ -1,4 +1,4 @@
#!/usr/bin/env python3
#!/usr/bin/env vpython3
Copy link
Contributor Author

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.

@mtolmacs
Copy link
Contributor Author

mtolmacs commented Dec 9, 2022

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

@loic-sharma
Copy link
Member

@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
  1. 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
...
  1. Update your pre-push hook
> gclient sync -D
Updating depot_tools...
...
Running hooks: 100% (10/10), done.
  1. 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
Copy link
Contributor

It is catching the formatting on my end

Copy link
Member

@cbracken cbracken left a 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?

@mtolmacs
Copy link
Contributor Author

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
Copy link
Member

loic-sharma commented Dec 16, 2022

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

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

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM stamp from a Japanese personal seal

@mtolmacs
Copy link
Contributor Author

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
Copy link
Member

loic-sharma commented Jan 3, 2023

@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
Copy link
Contributor Author

mtolmacs commented Jan 4, 2023

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 mtolmacs marked this pull request as draft January 4, 2023 11:23
@mtolmacs
Copy link
Contributor Author

mtolmacs commented Jan 5, 2023

For reference, this is the dartlang issue associated with this bug: dart-lang/sdk#50904

@Hixie
Copy link
Contributor

Hixie commented Apr 25, 2023

@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
Copy link
Contributor Author

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
Copy link
Member

/cc @brianquinlan in case he can think of any workarounds for dart-lang/sdk#50904 (desktop triage)

@loic-sharma
Copy link
Member

loic-sharma commented Jun 8, 2023

@mtolmacs if you rebase you should be unblocked now! The Dart SDK fix was rolled into the engine yesterday by ff7adb8.

in the Windows ci/format.bat instead of
//third_party/dart/tools/sdks/dart-sdk
@mtolmacs
Copy link
Contributor Author

(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
Copy link
Member

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
Copy link
Member

loic-sharma commented Jun 20, 2023

@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: flutter/flutter#129221

@mtolmacs
Copy link
Contributor Author

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.

Copy link
Member

@zanderso zanderso left a 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.

mtolmacs and others added 3 commits June 21, 2023 18:13
Co-authored-by: Zachary Anderson <[email protected]>
Co-authored-by: Zachary Anderson <[email protected]>
Co-authored-by: Zachary Anderson <[email protected]>
@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 21, 2023
@auto-submit auto-submit bot merged commit 08aaa88 into flutter:main Jun 21, 2023
@loic-sharma
Copy link
Member

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

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects: desktop autosubmit Merge PR when tree becomes green via auto submit App

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.