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.

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

patch-filters doesn't work on Windows OS #352

Closed
rnveach opened this issue Nov 13, 2022 · 9 comments
Closed

patch-filters doesn't work on Windows OS #352

rnveach opened this issue Nov 13, 2022 · 9 comments

Comments

@rnveach
Copy link
Member

rnveach commented Nov 13, 2022

Identified at checkstyle/eclipse-cs#395 (comment) ,

My local:

> git status
HEAD detached at z_Bananeweizen/patch_filter_not_working
nothing to commit, working tree clean

> git log -n 1
commit dfc3852f151313cc84ec10e04181084eb4178afc (HEAD, z_Bananeweizen/patch_filter_not_working)
Author: Michael Keppler <[email protected]>
Date:   Sun Nov 13 08:58:40 2022 +0100

    reproduce patch filter bug

    Auditor.java line 78 is the first violation reported by "mvn verify" if
    the patch filters are removed in the checkstyle_sevntu_checks.xml.

    So by adding a very long comment there we should _still_ see that same
    error, even if patch-filter is NOT removed.

> mvn clean verify
....
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for eclipse-cs Maven Parent 10.3.3-SNAPSHOT:
[INFO]
[INFO] eclipse-cs Maven Parent ............................ SUCCESS [  6.464 s]
[INFO] eclipse-cs Target Definition ....................... SUCCESS [  0.723 s]
[INFO] eclipse-cs Branding Plugin ......................... SUCCESS [  4.164 s]
[INFO] Checkstyle Core Library Plugin ..................... SUCCESS [ 20.956 s]
[INFO] eclipse-cs Core Plugin ............................. SUCCESS [  8.231 s]
[INFO] eclipse-cs Documentation Plugin .................... SUCCESS [  2.841 s]
[INFO] eclipse-cs UI Plugin ............................... SUCCESS [ 12.519 s]
[INFO] eclipse-cs Sample Plugin ........................... SUCCESS [  1.117 s]
[INFO] eclipse-cs Feature ................................. SUCCESS [  0.631 s]
[INFO] eclipse-cs Update Site ............................. SUCCESS [  2.995 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:23 min
[INFO] Finished at: 2022-11-13T09:20:57-05:00
[INFO] ------------------------------------------------------------------------

> type .\show.patch
diff --git a/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java b/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java
index cc6a8495..e23d4329 100644
--- a/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java
+++ b/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java
@@ -75,7 +75,7 @@ public class Auditor {
   private IProgressMonitor mMonitor;

   /** Map containing the file resources to audit. */
-  private final Map<String, IFile> mFiles = new HashMap<>();
+  private final Map<String, IFile> mFiles = new HashMap<>(); // CustomDeclarationOrder is raised here when the patch-filter is removed from checkstyle_sevntu_checks.xml

   /** Add the check rule name to the message. */
   private boolean mAddRuleName = false;

Passed with no errors.

Travis:
https://app.travis-ci.com/github/checkstyle/eclipse-cs/jobs/588314590#L625

[ERROR] src/net/sf/eclipsecs/core/builder/Auditor.java:[78,3] (extension) CustomDeclarationOrder: Field definition in wrong order. Expected 'Field(private final)' then 'Field(private.*)'.
[INFO] eclipse-cs Core Plugin ............................. FAILURE [ 15.526 s]

Reasoning for issue can be found in #113 (comment)

@rnveach
Copy link
Member Author

rnveach commented Nov 14, 2022

@Bananeweizen I have not fully confirmed eclipse-cs yet, but #113 is on the point to being resolved which is this repo's unit tests for Windows. Since eclipse-cs didn't get any exceptions, I am thinking the main culprit for Windows support was #113 (comment) and the slash handling of Audit events.

Please also be careful to note #363 as the current behavior of patch-filter in your repo is it won't include unstaged changes.

I will provide my report on eclipse-cs in another post, but I wanted to lay out the plan for even eclipse-cs to get this fix if it has been resolved. A plan is needed because patch-filters has already moved to CS 10.4 and I highly recommend sticking to the required version by sevntu which is still 10 otherwise we can't guarantee what will happen, as we have seen before.

  1. eclipse-cs needs to finish upgrading to CS 10.4 or the latest checkstyle version if more time is needed.
  2. sevntu will upgrade eclipse-cs version and CS version all to 10.4.
  3. sevntu will perform a release and at the same time patch-filters can release its Windows fixes as well. Patch-filters has no reliance on sevntu.
  4. eclipse-cs can upgrade all it's versions.

@rnveach
Copy link
Member Author

rnveach commented Nov 14, 2022

Patch Filters:

patch-filters> git log -n 1
commit f204f4d343833e786ead877169ccd5808ca65d27 (HEAD -> 113_last, pull/issue_113_last)
Author: rnveach
Date:   Mon Nov 14 13:38:05 2022 -0500

    Issue #113: resolves audit event matching on windows

patch-filters> git status
On branch 113_last
Your branch is up to date with 'pull/issue_113_last'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   pom.xml
        modified:   src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionJavaPatchFilterTest.java

no changes added to commit (use "git add" and/or "git commit -a")

patch-filters> mvn install
...
[INFO] --- maven-install-plugin:2.4:install (default-install) @ patch-filters ---
[INFO] Installing M:\checkstyleWorkspaceEclipse\patch-filters\target\patch-filters-1.3.1-SNAPSHOT.jar to C:\Users\rveac\.m2\repository\com\puppycrawl\tools\patch-filters\1.3.1-SNAPSHOT\patch-filters-1.3.1-SNAPSHOT.jar
[INFO] Installing M:\checkstyleWorkspaceEclipse\patch-filters\pom.xml to C:\Users\rveac\.m2\repository\com\puppycrawl\tools\patch-filters\1.3.1-SNAPSHOT\patch-filters-1.3.1-SNAPSHOT.pom
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  20.873 s
[INFO] Finished at: 2022-11-14T14:53:04-05:00
[INFO] ------------------------------------------------------------------------

Change in pom.xml was to temporarily downgrade to CS 10.0 . Change in SuppressionJavaPatchFilterTest was to disable failing test from downgrade.

Eclipse-CS:

eclipse-cs> git log -n 1
commit 0e7c21d75ff024693eb33bd892e783c331e3efc0 (HEAD)
Author: rnveach
Date:   Mon Nov 14 15:05:26 2022 -0500

    MY TEST

eclipse-cs> mvn verify
....
[INFO] --- maven-checkstyle-plugin:3.2.0:check (sevntu-checkstyle-check) @ net.sf.eclipsecs.core ---
[INFO] There is 1 error reported by Checkstyle 10.0 with config/checkstyle_sevntu_checks.xml ruleset.
[ERROR] src\net\sf\eclipsecs\core\builder\Auditor.java:[78,3] (extension) CustomDeclarationOrder: Field definition in wrong order. Expected 'Field(private final)' then 'Field(private.*)'.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for eclipse-cs Maven Parent 10.3.3-SNAPSHOT:
[INFO]
[INFO] eclipse-cs Maven Parent ............................ SUCCESS [  1.868 s]
[INFO] eclipse-cs Branding Plugin ......................... SUCCESS [  1.791 s]
[INFO] Checkstyle Core Library Plugin ..................... SUCCESS [ 11.986 s]
[INFO] eclipse-cs Core Plugin ............................. FAILURE [  5.212 s]

eclipse-cs> type .\show.patch
diff --git a/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java b/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java
index 5086dfe4..06d6a7a9 100644
--- a/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java
+++ b/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java
@@ -75,7 +75,7 @@ public class Auditor {
   private IProgressMonitor mMonitor;

   /** Map containing the file resources to audit. */
-  private final Map<String, IFile> mFiles = new HashMap<>();
+  private final Map<String, IFile> mFiles = new HashMap<>(); // CustomDeclarationOrder is raised here when the patch-filter is removed from checkstyle_sevntu_checks.xml

   /** Add the check rule name to the message. */
   private boolean mAddRuleName = false;
diff --git a/pom.xml b/pom.xml
index 6843aeae..b7b01b5b 100644
--- a/pom.xml
+++ b/pom.xml
@@ -26,7 +26,7 @@
         <!-- sevntu and patch-filters need to use a compatible/compiled version with checkstyle -->
         <maven.sevntu.checkstyle.plugin.checkstyle.version>10.0</maven.sevntu.checkstyle.plugin.checkstyle.version>
         <maven.sevntu.checkstyle.plugin.version>1.42.0</maven.sevntu.checkstyle.plugin.version>
-        <maven.patch.filters.plugin.version>1.2.0</maven.patch.filters.plugin.version>
+        <maven.patch.filters.plugin.version>1.3.1-SNAPSHOT</maven.patch.filters.plugin.version>
         <!-- see above -->
     </properties>

I am satisfied that the changes under #113 resolve this issue.

@Bananeweizen
Copy link

What I can say is that the exceptions in the patch-filters unit tests are definitely gone for me. Beyond that, the Locale issue that we just discussed in the main project stops me from confirming that it works. But I have trust in you solving it fully.

@rnveach
Copy link
Member Author

rnveach commented Nov 15, 2022

@romani I added windows run at #362 . It is waiting on my other pr to be merged before it's green.

@romani
Copy link
Member

romani commented Nov 16, 2022

@rnveach , is this issue resolved by
#113
#361 ?

@rnveach
Copy link
Member Author

rnveach commented Nov 16, 2022

Correct. I provided results at #352 (comment) that it works as reported.

@romani
Copy link
Member

romani commented Nov 16, 2022

Released in 1.4.0

@romani romani closed this as completed Nov 16, 2022
@romani
Copy link
Member

romani commented Dec 1, 2022

@Bananeweizen , are you ok to try #363 (comment) in this repo ?

@Bananeweizen
Copy link

Are you asking, whether I would run the command for adding the files from that comment manually on changes? If so, no. I know that I would forget this all the time, since I don't even use the command line for my git operations (I use egit).
From my experience with the patch-filters over the previous 2 weeks I'm completely fine with getting the results only for already committed files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants