MetadataGeneratorUtilTest is modifying files in the main folder for Windows hosts

[rnveach]

Identified when backporting 8.36.1 . Confirmed this still happens in master.

Running MetadataGeneratorUtilTest is modifying 63 files in src/main/resources/com/puppycrawl/tools/checkstyle/meta/. This happens when running by itself or as part of the suite.

No test should ever modify our files controlled by git. Especially production code/resources. This is an accident waiting to be staged and committed.

Here is the patch of one of the modifications.

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
index 82e64e5..4efd0de 100644
--- a/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml
+++ b/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/ArrayTypeStyleCheck.xml
@@ -4,18 +4,18 @@
       <check fully-qualified-name="com.puppycrawl.tools.checkstyle.checks.ArrayTypeStyleCheck"
              name="ArrayTypeStyle"
              parent="com.puppycrawl.tools.checkstyle.TreeWalker">
-         <description>&lt;p&gt;
- Checks the style of array type definitions.
- Some like Java style: {@code public static void main(String[] args)}
- and some like C style: {@code public static void main(String args[])}.
- &lt;/p&gt;
- &lt;p&gt;
- By default the Check enforces Java style.
- &lt;/p&gt;
- &lt;p&gt;
- This check strictly enforces only Java style for method return types regardless
- of the value for 'javaStyle'. For example, {@code byte[] getData()}.
- This is because C doesn't compile methods with array declarations on the name.
+         <description>&lt;p&gt;&#xD;
+ Checks the style of array type definitions.&#xD;
+ Some like Java style: {@code public static void main(String[] args)}&#xD;
+ and some like C style: {@code public static void main(String args[])}.&#xD;
+ &lt;/p&gt;&#xD;
+ &lt;p&gt;&#xD;
+ By default the Check enforces Java style.&#xD;
+ &lt;/p&gt;&#xD;
+ &lt;p&gt;&#xD;
+ This check strictly enforces only Java style for method return types regardless&#xD;
+ of the value for 'javaStyle'. For example, {@code byte[] getData()}.&#xD;
+ This is because C doesn't compile methods with array declarations on the name.&#xD;
  &lt;/p&gt;</description>
          <properties>
             <property default-value="true" name="javaStyle" type="boolean">

[rnveach]

ping @romani @gaurabdg

[rnveach]

I don't understand the purpose of running MetadataGeneratorUtil.generate. As the name suggests, it is the culprit and sounds like it is just something to be run to update checkstyle with a release. Not to be run with a test on every unrelated change.

If this is not to be run as part of the suite, it needs extra safe guards to prevent this issue. See https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java#L44-L46 as an example.

Otherwise, it will be a lot more complex to ensure it doesn't randomly change the files when no changes are being made. This would still make this an accident waiting to happen. I would look into changing the test from a write to a compare like the xdocs/javaxdocs.

[romani]

It was part of design decision that we did to make we update file if metada is changing. It worked ok for now, I did not have any problems with it.

[rnveach]

Since this is part of the test phase and not the compile phase (or something similar to updating production code) I worry that this will cause some unintended issue like above. I am just pointing out the issue.

There is no guarantee that someone will run the tests when making updates. Is there a CI check to ensure this is verified?

Is this issue approved to atleast fix this current issue?

[romani]

Is there a CI check to ensure this is verified?

Yes. Travis. https://github.com/checkstyle/checkstyle/blob/32de8909329d4d3f19a3c05f5c5deefe6ffd0b35/.travis.yml#L349

But I agree that we planned to make it to be done on phase of sources generation. Current behavior is temporal behavior.

[rnveach]

Yes. Travis

What about appveyor? I am on Windows after all.

[romani]

Does it work differently ? Do you have changes in files after clone and build ?

[rnveach]

Do you have changes in files after clone and build ?

Yes. See first post. Thats why I started this issue. I would have never known about the modifying production files otherwise.

[romani]

looks like we have problems with lineends

- Checks the style of array type definitions.
....
+ Checks the style of array type definitions.


Project has enforced Linux lineends even in Windows OS after clone https://github.com/checkstyle/checkstyle/blob/32de8909329d4d3f19a3c05f5c5deefe6ffd0b35/.gitattributes#L1-L2

but metadata generator make files with Windows native lineends, we need to force xml writer to use linux lineends.

@gaurabdg , please help to fix issue.

[romani]

solution might be https://stackoverflow.com/a/42582639/1015848 to update code at https://github.com/checkstyle/checkstyle/blob/cd6a36022053d032c9c0bce6e47f4cb2d2707ac5/src/main/java/com/puppycrawl/tools/checkstyle/meta/XmlMetaWriter.java#L177-L184

[anhminhtran235]

I am on it

[anhminhtran235]

@rnveach @romani I'm using Windows 10. When I ran MetadataGeneratorUtilTest.java, no files are changed. Here were the steps I took. Please tell me if I did something wrong:

1/ Make sure my local is up to date with checkstyle master. 2/ Run checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\meta\MetadataGeneratorUtilTest.java 3/ Type git status 4/ Git status result:

On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

[romani]

@anhminhtran235 , while we waiting for reply, can you update appveyor config https://github.com/checkstyle/checkstyle/blob/master/appveyor.yml To do same validation as Travis does https://github.com/checkstyle/checkstyle/blob/608389aaf407ccc3ee38b209829dd550f2355ee4/.travis.yml#L353 If after buid status of git is not clean, fail a build

[anhminhtran235]

@romani I'm not sure how to write .yml code. Also, do you want me to update appveyor config locally or push to checkstyle master?

[romani]

Can you write powershell command that return non zero exit code if status in fit is not empty https://github.com/checkstyle/checkstyle/blob/608389aaf407ccc3ee38b209829dd550f2355ee4/appveyor.yml#L81 And we just need to add such command to be placed in yml.

[anhminhtran235]

I've never written powershell command before. I don't quite understand how powershell and appveyor.yml work. Do you want me to learn it and then add the command, or do you want people who know powershell do it?

If you want me to learn it, please suggest a few sources where I can learn about appveyor.yml and powershell.

[romani]

@anhminhtran235 , please skip this issue for now. @rnveach , we do need your help here, as do not have anybody who can reproduce it. It would be awesome if you can reproduce it in CI.

[kalpadiptyaroy]

I am starting to work on it.

[rnveach]

@kalpadiptyaroy Why don't we finish https://github.com/checkstyle/checkstyle/pull/13696 first to help prove the issue and your changes work.

[kalpadiptyaroy]

I have written some code using LSSerializer. But I am not sure how to test this. Can I get some hints regarding this? Precisely, I want to know is this MetadataGeneratorUtilTest is disabled or suppressed? if yes then, how to enable it?

[kalpadiptyaroy]

@kalpadiptyaroy Why don't we finish #13696 first to help prove the issue and your changes work.

This is done from my end. Just awaiting the merge.

[rnveach]

Then I would split https://github.com/checkstyle/checkstyle/pull/13696 so it just the git-diff, as I don't agree with approving the rest as things stand.

As for MetadataGeneratorUtilTest , it shouldn't be suppressed. If your git diff doesn't show this issue, then I can only assume there is some other conflict between local and CI that it shows for me.

[kalpadiptyaroy]

@rnveach When, I execute mvn clean verify in my local (windows) then only diff occurred was for Xdocs!

Hence, I am a bit confused whether this test is disabled or is there something else.

[rnveach]

There must be something else like I said. This is still an issue for me.

[kalpadiptyaroy]

Then I would split https://github.com/checkstyle/checkstyle/pull/13696 so it just the git-diff, as I don't agree with approving the rest as things stand.

Kindly come to a conclusion on PR #13696 regarding split. I mean a reply on the PR thread will ease out the situation. Then I would remove the cherry picked commit till the time we decide how to set custom newline in Doxia officially!

[romani]

We will go step by step, if this issue is not resolved by our PRs, it should stay open until somebody comeup with solution.

[kalpadiptyaroy]

There must be something else like I said. This is still an issue for me.

You are right! I executed this test a couple of times in my windows 10 local env. But no such git diff occurred. Surely something is not right in your local. Let's wait for someone else who could reproduce it!

So, I am not taking this up as of now!

[romani]

At least we fixed it in CI so windows users will be covered going forward a way more than they are now, so we doing good step forward, even there is still something that doesn't work for all.