Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: Files without newline seem to fail at applying patch #487

Open
fzakaria opened this issue Feb 19, 2025 · 6 comments · May be fixed by #492
Open

[Bug]: Files without newline seem to fail at applying patch #487

fzakaria opened this issue Feb 19, 2025 · 6 comments · May be fixed by #492
Labels
bug Something isn't working

Comments

@fzakaria
Copy link
Contributor

fzakaria commented Feb 19, 2025 •
edited
Loading

What happened?

Lint results for //tools/lint:SortImportOrder:

Some problems have automated fixes available:

  tools/lint/SortImportOrder.java | 4 ++--
  1 file, 2 insertions(+), 2 deletions(-)

Error: failed to apply patch file for //tools/lint:SortImportOrder: conflict: fragment line does not match src line

If we build it manually

> bazel build //tools/lint:SortImportOrder  --aspects //tools/lint:linters.bzl%sort_imports --output_groups=rules_lint_patch --@aspect_rules_lint//lint:fix  --remote_download_all=true

> cat  bazel-bin/tools/lint/SortImportOrder.AspectRulesLintKeepJavaImportsSorted.patch
--- a/tools/lint/SortImportOrder.java
+++ b/tools/lint/SortImportOrder.java
@@ -1,14 +1,14 @@
 package tools.lint;

 import java.io.OutputStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.Scanner;
 import java.util.Set;
\ No newline at end of file

That patch seems to be messed up if we apply it with git apply
(notice lack of newline after java.util.Set)

> git patch tools
diff --git a/tools/lint/SortImportOrder.java b/tools/lint/SortImportOrder.java
index 35a3e87bdc2..0bb6c25fc47 100644
--- a/tools/lint/SortImportOrder.java
+++ b/tools/lint/SortImportOrder.java
@@ -1,17 +1,17 @@
 package tools.lint;

 import java.io.OutputStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.Scanner;
-import java.util.Set;
-import java.util.TreeSet;
+import java.util.Set;import java.util.TreeSet;
 import java.util.stream.Collectors;

If we apply a newline at the end of our file; our patch file is slightly different

> cat bazel-bin/tools/lint/SortImportOrder.AspectRulesLintKeepJavaImportsSorted.patch
--- a/tools/lint/SortImportOrder.java
+++ b/tools/lint/SortImportOrder.java
@@ -1,14 +1,14 @@
 package tools.lint;

 import java.io.OutputStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.Scanner;
 import java.util.Set;

It is missing \ No newline at end of file in the patch.
This patch when applied produces the correct results

Version

> cat .bazeliskrc
BAZELISK_BASE_URL=https://static.aspect.build/aspect
USE_BAZEL_VERSION=aspect/2024.51.11

http_archive(
    name = "aspect_rules_lint",
    sha256 = "f60e4a737a5e09402f5fa3bd182efa80dac5523ca4b9bc5c6fa8c06fbfb46630",
    strip_prefix = "rules_lint-1.1.0",
    url = "https://github.com/aspect-build/rules_lint/releases/download/v1.1.0/rules_lint-v1.1.0.tar.gz",
)

How to reproduce

I was able to reproduce it locally with the linters already setup.
I changed Bar.java

> git patch
diff --git a/example/src/Bar.java b/example/src/Bar.java
index b1ea6f4..e9a5825 100644
--- a/example/src/Bar.java
+++ b/example/src/Bar.java
@@ -10,11 +10,11 @@ public class Bar {
     // keep-sorted start
     B(),
     A(),
-    D(),
     C(),
+    D(),
     // keep-sorted end
   }

   // Max line length set to 20, so this should raise issue.
   protected void finalize(int a) {}
-}
+}
\ No newline at end of file
  • Make the keep-sorted changed to fail
  • remove new line
INFO: From Linting //src:bar with Checkstyle:
Checkstyle ends with 5 errors.
INFO: From Linting //src:foo with Checkstyle:
Checkstyle ends with 21 errors.
INFO: Found 26 targets...
INFO: Elapsed time: 364.087s, Critical Path: 8.10s
INFO: 1089 processes: 424 disk cache hit, 616 internal, 48 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 1089 total actions
Lint results for //src:bar:

Some problems have automated fixes available:

  src/Bar.java | 2 +-
  1 file, 1 insertion(+), 1 deletion(-)

Error: failed to apply patch file for //src:bar: conflict: fragment line does not match src line

Any other information?

No response
@fzakaria fzakaria added the bug Something isn't working label Feb 19, 2025
@alexeagle
Copy link
Member

Could you please fill out the "How to reproduce" prompt? I'm not able to reproduce this, I think whatever tool is backing the SortImportOrder linter you mentioned is required to produce the malformed patch file.

@fzakaria
Copy link
Contributor Author

@alexeagle did you try with a file without a newline?

I was able to reproduce it locally with the linters already setup.
I changed Bar.java

> git patch
diff --git a/example/src/Bar.java b/example/src/Bar.java
index b1ea6f4..e9a5825 100644
--- a/example/src/Bar.java
+++ b/example/src/Bar.java
@@ -10,11 +10,11 @@ public class Bar {
     // keep-sorted start
     B(),
     A(),
-    D(),
     C(),
+    D(),
     // keep-sorted end
   }

   // Max line length set to 20, so this should raise issue.
   protected void finalize(int a) {}
-}
+}
\ No newline at end of file
  • Make the keep-sorted changed to fail
  • remove new line
INFO: From Linting //src:bar with Checkstyle:
Checkstyle ends with 5 errors.
INFO: From Linting //src:foo with Checkstyle:
Checkstyle ends with 21 errors.
INFO: Found 26 targets...
INFO: Elapsed time: 364.087s, Critical Path: 8.10s
INFO: 1089 processes: 424 disk cache hit, 616 internal, 48 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 1089 total actions
Lint results for //src:bar:

Some problems have automated fixes available:

  src/Bar.java | 2 +-
  1 file, 1 insertion(+), 1 deletion(-)

Error: failed to apply patch file for //src:bar: conflict: fragment line does not match src line

@fzakaria fzakaria mentioned this issue Mar 4, 2025
Open
@fzakaria
Copy link
Contributor Author

fzakaria commented Mar 4, 2025

I tried bumping the version but latest (0.8.1) still has the bug bluekeyes/go-gitdiff#57

@fzakaria
Copy link
Contributor Author

fzakaria commented Mar 4, 2025

Okay I did some debugging with @vinnybod -- looks to be an issue where BSD diff will produce diffs with multiple \ No newline at end of file

This causes bugs in go-gitdiff and git itself (we sent an email to their mailing list).

Switching to GNU diff fixes the problem but how can we get around this in rules_lint?

Ideas 💡

  • go-gitdiff itself can generate patches; should we leverage that instead ?
  • check if git exists before using diff ?
  • check if diff --version says "BSD" and print a friendly warning?

@alexeagle
Copy link
Member

I think we just need to resolve this TODO
https://github.com/aspect-build/rules_lint/blob/main/lint/private/patcher.mjs#L91
so we can rely on having GNU diff (or any specific non-buggy diff implementation)

@alexeagle
Copy link
Member

https://gitlab.arm.com/bazel/ape can give us a GNU diff binary, which I think is a reasonable way to wire this up.

fzakaria added a commit to fzakaria/rules_lint that referenced this issue Mar 5, 2025
@fzakaria
Swap diff tool for git
0326eac 
go-gitdiff which aspect CLI uses cannot support patches generated by
BSD diff which is the default on MacOS (see tagged issue).

I am investigating using ape but it requires Bazel 7.4.1 which may be
a breaking change.

Alternatively, this changes diff -> git with a few extra "hacks".
Namely, we need to replace the labels to be the same file and resolve
the symlinks otherwise `git diff` thinks the file was deleted.

fixes aspect-build#487
@fzakaria fzakaria linked a pull request Mar 5, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Swap diff tool for git fzakaria/rules_lint
2 participants
@alexeagle @fzakaria