Issue #11816: Added windows cmd script for git-diff validation by kalpadiptyaroy · Pull Request #13696 · checkstyle/checkstyle

kalpadiptyaroy · 2023-09-04T18:49:27Z

Issue #11816: Added cmd script for git-diff validation for windows environment

romani

For testing , create second commit for same PR where you remove any ord at

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml

Line 8 in 9820e86

Checks the style of array type definitions.

It should trigger git diff after mvn test

Items to improve:

kalpadiptyaroy · 2023-09-05T18:26:09Z

For testing , create second commit for same PR where you remove any ord at

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml

Line 8 in 9820e86

Checks the style of array type definitions.

It should trigger git diff after mvn test
Items to improve:

Done

romani · 2023-09-06T02:14:47Z

CI does not like your update https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47962839

please show textual copy of terminal output from your local to show how it works, you can change any file you want (without commit) and run a script. Two outputs are required with error and without error. Please also show that in case of diff script return error code to let CI fail.

kalpadiptyaroy · 2023-09-06T18:36:22Z

CI does not like your update https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47962839

please show textual copy of terminal output from your local to show how it works, you can change any file you want (without commit) and run a script. Two outputs are required with error and without error. Please also show that in case of diff script return error code to let CI fail.

I am trying out something. If it doesn't work. I will do as per your suggestion

kalpadiptyaroy · 2023-09-08T19:11:53Z

@romani. This is the output you asked for.

D:\opensource\project-checkstyle\checkstyle>".ci/validation.cmd" git_diff
Please clean up or update .gitattributes file.
Git status output:    
Top 300 lines of diff:
[1]diff --git a/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml b/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml
[2]index 124bf3c13..5698bd438 100644
[3]--- a/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml
[4]+++ b/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml
[5]@@ -5,7 +5,7 @@
[6]              name="ArrayTypeStyle"
[7]              parent="com.puppycrawl.tools.checkstyle.TreeWalker">
[8]          <description>&lt;p&gt;
[9]- Checks the style of array type definitions for test.
[10]+ Checks the style of array type definitions.
[11]  Some like Java style: {@code public static void main(String[] args)}
[12]  and some like C style: {@code public static void main(String args[])}.
[13]  &lt;/p&gt;
validation failed with error code #-1.

D:\opensource\project-checkstyle\checkstyle>git status
On branch windows-cmd-validations-script
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml

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

D:\opensource\project-checkstyle\checkstyle>git checkout -- .

D:\opensource\project-checkstyle\checkstyle>git status
On branch windows-cmd-validations-script
nothing to commit, working tree clean

D:\opensource\project-checkstyle\checkstyle>".ci/validation.cmd" git_diff
Please clean up or update .gitattributes file.
Git status output:
Top 300 lines of diff:
validation failed with error code #-1.

D:\opensource\project-checkstyle\checkstyle>

romani

Items

romani · 2023-09-09T21:10:07Z

https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47995477/job/k9s9pby4sq58vbfl#L339

[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:55 min
[INFO] Finished at: 2023-09-09T15:18:55Z
[INFO] ------------------------------------------------------------------------
CMD exited with exit code 0
./.ci/validation.cmd git_diff
validation failed with error code #1.
Command exited with code 1

lets improve error message to make it clear for any new contributor on what is wrong and what should be fixed.

please make it something like this:

./.ci/validation.cmd git_diff
There is git changes after execution of job:
<print whole diff as is> 
There should be no changes in git repository after any CI job/task
validation failed with error code #1.

kalpadiptyaroy · 2023-09-10T06:28:56Z

https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47995477/job/k9s9pby4sq58vbfl#L339

[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:55 min
[INFO] Finished at: 2023-09-09T15:18:55Z
[INFO] ------------------------------------------------------------------------
CMD exited with exit code 0
./.ci/validation.cmd git_diff
validation failed with error code #1.
Command exited with code 1

lets improve error message to make it clear for any new contributor on what is wrong and what should be fixed.

please make it something like this:

./.ci/validation.cmd git_diff
There is git changes after execution of job:
<print whole diff as is> 
There should be no changes in git repository after any CI job/task
validation failed with error code #1.

Sure. On it. Will be pushing changes soon!

romani

~~it should be failed on first job - https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47997876/job/d8kr55f48af3um3j#L344 , but it is not failed.~~ it is not validation that is not chaning files - all good.

Items

kalpadiptyaroy · 2023-09-10T14:13:55Z

it should be failed on first job - https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47997876/job/d8kr55f48af3um3j#L344 , but it is not failed.

Items

@romani
https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47998600/job/32id656n29ij6ip3

Have a look. I made some changes and now its failing in appveyor. Kindly check if it meets the expectations.

romani · 2023-09-10T14:14:02Z

https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47998600/job/32id656n29ij6ip3#L1862 this is supper scary failure that is hard to understand even for me.

can we avoid this warning on line ends?

romani

item

kalpadiptyaroy · 2023-09-10T14:20:30Z

https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47998600/job/32id656n29ij6ip3#L1862 this is supper scary failure that is hard to understand even for me.

can we avoid this warning on line ends?

Actually, these warnings are generated because the mvn build OR git is somehow modifying the EOF characters from CRLF to LF. I am not sure, how to handle these warnings as of now.

But I feel that something similar to issue #8884 is happening here!

If you can provide me some hints and clues. I might try them out!

kalpadiptyaroy · 2023-10-08T18:59:25Z

Done

rnveach

PR is blocked by CI until #13774 .

romani · 2023-10-23T13:31:11Z

@kalpadiptyaroy, please remove second commit and rebase, we are very close to finish line.

romani · 2023-10-23T15:14:30Z

Hmm, we still have problems with line endings
https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/48344118/job/kpdxe2nvufjol2dv#L1866

Did we missed something ?

kalpadiptyaroy · 2023-10-23T18:28:36Z

@romani - Yes, we did miss!

I debugged the entire process once again and the findings are following.

See the XdocTemplateSink class calls the writeEOL func. as shown below.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/site/XdocsTemplateSink.java

Lines 59 to 62 in 0f9d09e

    
           public void body() { 
        
               write("<?xml version=\"1.0\" encoding=\"" + encoding + "\"?>"); 
        
               writeEOL(); 
        
           }

Going into writeEOL we can see a chain of calls to the write(String) func. that bottoms out to:
https://github.com/apache/maven-doxia/blob/e2ae1195cc28d70f6f74c5aed5384b031dc92933/doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/Xhtml5BaseSink.java#L2006-L2014

Now the write(String) func that is taking the unified EOL string is finally implemented as: https://github.com/openjdk/jdk/blob/8d9a4b43f4fff30fd217dab2c224e641cb913c18/src/java.base/share/classes/java/io/PrintWriter.java#L634-L636
AND
https://github.com/openjdk/jdk/blob/8d9a4b43f4fff30fd217dab2c224e641cb913c18/src/java.base/share/classes/java/io/PrintWriter.java#L602-L616

So final conclusion is we should have overriden write(String) func of PrintWriter as follows rather than the println() func. because println is never used in this case.

@Override
public void write(String x) {
      ...... // use replace func or another logic to convert all \r\n and \r to \n
}

kalpadiptyaroy · 2023-10-23T18:36:23Z

Also, I am quite surprised that Apache Doxia is only using write func to do everything and it is not utilizing the other Writer functions such as println or print.

Now we have to come to concrete decision. Since we are left with 2 ways.

Re-open issue Enforce Linux Style End-of-Line (LF instead of CRLF) in all XML files for windows environment. #13770 and override write func. in CustomPrintWriter. OR
We have to ask Doxia community to change their entire code structure to utilize the respective functions of Writer in a systematic way.

Let me know your thoughts!

I know option 1 is the easiest as of now - but still other maintainers' opinion also matters!

romani · 2023-10-23T18:46:07Z

Issue is reopened on our side.
Let's do what we need now to make it work and do not wait for doxia to change code.

We can share feedback at https://issues.apache.org/jira/browse/DOXIA-707 for them to improve.

kalpadiptyaroy · 2023-10-23T18:46:32Z

Issue is reopened on our side. Let's do what we need now to make it work and do not wait for doxia to change code.

Cool

kalpadiptyaroy · 2023-10-24T17:58:27Z

@romani - Its working after cherry picking the #13947 commit here.

romani · 2023-10-24T18:26:41Z

All CIs are green, confirmed.
https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/48355264

Please remove commit.

kalpadiptyaroy · 2023-10-25T14:47:26Z

Appveyor Test is passed. We are good to merge.

Only closed issue reference check has failed. I don't know why.
@romani - can this be ignored?

romani · 2023-10-25T15:41:33Z

@rnveach , please finalize review

rnveach · 2023-10-26T16:29:38Z

 )

+if "%OPTION%" == "git_diff" (
+  for /f "delims=" %%a in ('git status ^| findstr /c:"Changes not staged"') do set output=%%a


Just pointing out that this command relies on the english locale.

So is there any locale compliance that I haven't complied with?

I have tried to keep it at par with

https://github.com/checkstyle/checkstyle/blob/master/.ci/validation.sh#L1078-L1086

We have a lot of scripts that rely on English locale. We can presume for now that CI will be English only for next few years for sure

Ok. Let's merge then.

nrmancuso · 2023-10-28T18:56:35Z

@kalpadiptyaroy thanks for all your work on this, it is a big win for windows users.

kalpadiptyaroy · 2023-10-30T06:40:50Z

@kalpadiptyaroy thanks for all your work on this, it is a big win for windows users.

Welcome Nick. This appreciation is a great motivation for me.

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch from 1b02881 to b3d1666 Compare September 4, 2023 18:49

romani requested changes Sep 5, 2023

View reviewed changes

Comment thread appveyor.yml Outdated

Comment thread .ci/validation.cmd Outdated

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch from b3d1666 to 45b3c57 Compare September 5, 2023 18:24

nrmancuso reviewed Sep 5, 2023

View reviewed changes

Comment thread appveyor.yml Outdated

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch from aa0bb09 to 043454b Compare September 6, 2023 17:35

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 2 times, most recently from 328c45a to 53fb91f Compare September 7, 2023 15:51

romani requested changes Sep 9, 2023

View reviewed changes

Comment thread appveyor.yml Outdated

nrmancuso assigned romani Sep 9, 2023

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 2 times, most recently from 22fb982 to 557833b Compare September 9, 2023 15:12

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 4 times, most recently from 4d3b72d to 88b382e Compare September 10, 2023 08:37

romani requested changes Sep 10, 2023

View reviewed changes

Comment thread appveyor.yml Outdated

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 2 times, most recently from fdac749 to 044e832 Compare September 10, 2023 13:25

romani requested changes Sep 10, 2023

View reviewed changes

Comment thread .ci/validation.cmd

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 2 times, most recently from 147da08 to 3a36615 Compare September 10, 2023 14:28

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch from 8c39cfe to a0d7c33 Compare October 8, 2023 18:15

rnveach suggested changes Oct 8, 2023

View reviewed changes

rnveach added the blocked label Oct 12, 2023

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 2 times, most recently from a0d7c33 to d9e9461 Compare October 23, 2023 13:40

kalpadiptyaroy force-pushed the windows-cmd-validations-script branch 3 times, most recently from 5c674df to 937f27f Compare October 25, 2023 13:42

Issue checkstyle#11816: Added windows cmd script for git-diff validation

284bd1d

romani force-pushed the windows-cmd-validations-script branch from 1e42650 to 284bd1d Compare October 25, 2023 15:40

romani removed the blocked label Oct 25, 2023

rnveach approved these changes Oct 26, 2023

View reviewed changes

rnveach assigned nrmancuso and unassigned rnveach Oct 26, 2023

nrmancuso approved these changes Oct 28, 2023

View reviewed changes

nrmancuso merged commit 281fd0e into checkstyle:master Oct 28, 2023

nrmancuso mentioned this pull request Oct 28, 2023

create validation of git status on Windows environments #11816

Closed

Uh oh!

Conversation

kalpadiptyaroy commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kalpadiptyaroy commented Sep 5, 2023

Uh oh!

Uh oh!

romani commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalpadiptyaroy commented Sep 6, 2023

Uh oh!

kalpadiptyaroy commented Sep 8, 2023

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani commented Sep 9, 2023

Uh oh!

kalpadiptyaroy commented Sep 10, 2023

Uh oh!

romani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kalpadiptyaroy commented Sep 10, 2023

Uh oh!

romani commented Sep 10, 2023

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kalpadiptyaroy commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalpadiptyaroy commented Oct 8, 2023

Uh oh!

rnveach left a comment

Choose a reason for hiding this comment

Uh oh!

romani commented Oct 23, 2023

Uh oh!

romani commented Oct 23, 2023

Uh oh!

kalpadiptyaroy commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalpadiptyaroy commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalpadiptyaroy commented Oct 23, 2023

Uh oh!

kalpadiptyaroy commented Oct 24, 2023

Uh oh!

romani commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalpadiptyaroy commented Oct 25, 2023

Uh oh!

romani commented Oct 25, 2023

Uh oh!

rnveach Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

kalpadiptyaroy Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalpadiptyaroy commented Sep 4, 2023 •

edited

Loading

romani commented Sep 6, 2023 •

edited

Loading

romani left a comment •

edited

Loading

kalpadiptyaroy commented Sep 10, 2023 •

edited

Loading

kalpadiptyaroy commented Oct 23, 2023 •

edited

Loading

kalpadiptyaroy commented Oct 23, 2023 •

edited

Loading

romani commented Oct 23, 2023 •

edited

Loading

romani commented Oct 24, 2023 •

edited

Loading

kalpadiptyaroy Oct 26, 2023 •

edited

Loading