maven-checkstyle-plugin
maven-checkstyle-plugin copied to clipboard
apache
[MCHECKSTYLE-376] Remove window line endings from files
mvn verify and mvn -Prun-its clean verify passed.
Found these doing find . -type f -name '*.*' -exec dos2unix '{}' + on linux.
So this is your fix for MCHECKSTYLE-376. Can you please change the commit message and the itle of the PR, this way we will automagically link your patch to the issue.
I was fixing only that java file here https://github.com/apache/maven-checkstyle-plugin/tree/MCHECKSTYLE-376
but your approach is perhaps better, so I approve it thank you
So your are changing all to LF ? In my fix I only changed Mcheckstyle54.java to CFLF.
Honestly I don't know if we have a rule for line endings. We should enforce it on every maven repository.
I am fine with LF only
cc @olamy @rfscholte @khmarbaise @hboutemy @Tibor17
Will Windows users (developers of the plugin I mean) be happy with this change? Or is it better to have CRLF every where?
I have amended the commit message and force pushed to https://github.com/apache/maven-checkstyle-plugin/tree/MCHECKSTYLE-376 so my comment above is no more valid
Let's see CI https://builds.apache.org/job/maven-box/job/maven-checkstyle-plugin/job/MCHECKSTYLE-376/2/
@eolivelli Sorry for the confusion and not giving enough details in my first post. I thought this would have been clearer. This is in response to MCHECKSTYLE-376 but is not a fix. This is why I didn't put the issue number in the commit message. None of the files in MCHECKSTYLE-376 were modified in this PR. I was just merely making all line endings the same in the repo. So the issue still remains for MCHECKSTYLE-376. Like I amended in the issue, I looked at the raw files reported and I saw nothing wrong, so I believe the issue to be how it is cloned.
Will Windows users be happy with this change? Windows users should be fine thanks to Git autocrlf.
I just recommend documenting this somewhere so it is clear that it is required for windows users. As this PR demonstrates, there are no checks to ensure future files remain with 1 line ending.
I just recommend documenting this somewhere so it is clear that it is required for windows users.
I wouldn’t say it was required. Unless people are opening source files in Notepad, it should be fine. Modern IDEs and text editors work fine with unix line endings on Windows.
Not all ITs use the same rules.
Only some ITs use the problematic default rules, and there you have to write the Groovy script like unix2dos on Windows. Modern IDEs like IDEA respect:
- Git settings
- if not exists, then the exactly EOL specified in settings.
I don't like very much the groovy script that rewrites a source file. This is not safe, because those .java files are the sources of tests, so when you run the test you are changing the code and make the git dirty (you will see source files changed with 'git status').
I think the original issue of "MCHECKSTYLE-54 checkstyle:check does not see provided scope dependencies" has nothing to do with EOL. We can just fix the test by committing a fixed version of the .java file. That integration test should only test the case of the issue.
@rnveach what about adding in this commit a fix for Mcheckstyle54.java ? That test is using the default sun_checks.xml, which has the offending rule. We could modify the integration test in a way that it does not use the default checkstyle configuration file.
@eolivelli Then let's wait for the fix in checkstyle dependency and we don't have to do anything after this PR. This PR is good but i haven't used it on my system.
@eolivelli I mean https://github.com/checkstyle/checkstyle/pull/6559 One commit has been already pushed https://github.com/checkstyle/checkstyle/commit/d2a53a9818b46cc188b8a17cf6e7e9d19038db67 but the checkstyle team can say more.
@Tibor17 I see. Thanks. Let's wait for the fix on checkstyle. Them we will commit this patch and the upgrade of checkstyle to latest and greatest version
Modern IDEs and text editors work fine with unix line endings on Windows.
Yes, it isn't a problem with working an existing file as long as the IDE keeps the same line encoding. When creating a new file in the project and crlf turned off, my Eclipse Oxygen created the file with \r\n line endings while all the other files in the repo were \n. As shown in this PR, there wasn't a \n in randomly places in any of the files, but it was the whole file that was using \r\n. It can't hurt, imo, to notify new developers of this requirement.
what about adding in this commit a fix for Mcheckstyle54.java ? That test is using the default sun_checks.xml, which has the offending rule.
While looking at https://issues.apache.org/jira/browse/MCHECKSTYLE-54 it appears the issue was originally with JavadocMethodCheck as it is the only AbstractTypeAwareCheck that loads classinformation. A simple fix is to use a custom configuration with only that check in it and that way we don't care about new lines.
a fix for Mcheckstyle54
Done and added the issue number to the commit message.
Just as a note, this patches the issue to pass it, but it doesn't fix it. Somewhere in your process, the files are not in the system's default line endings and it could present an issue in the future in some way, like if you need to add an IT specifically about the new lines.
@rnveach I have pushed again this branch to ASF repo
Waiting for CI, then I am merging this one https://builds.apache.org/job/maven-box/job/maven-checkstyle-plugin/job/MCHECKSTYLE-376/
We can create an issue for the upgrade to latest checkstyle
@rnveach it seems that we have a couple of failures, see
https://builds.apache.org/job/maven-box/job/maven-checkstyle-plugin/job/MCHECKSTYLE-376/3/console
can you help me investigate ?
The build is failing on every windows machine. I am trying to update checkstyle dep now to latest and greatest
@eolivelli
@rnveach
Now the Git does not change the EOL character in Jekins build. No changes of text files by Git settings.
What happens if the checkstyle version would be 8.21?
[ERROR] The following builds failed: [ERROR] * MCHECKSTYLE-214-basedir-resource\pom.xml [ERROR] * MCHECKSTYLE-70-multi-sourcefolder\pom.xml
These are the files I changed in this PR so this is the same issue related to MCHECKSTYLE-54. I would have to look into these issues to see what I can change the configurations to. I can revert these files and only keep the changes only for MCHECKSTYLE-54 in this PR and move the line endings to another PR.
I am running mvn verify -P run-its. Two ITs failed:
MCHECKSTYLE-214: You have 4 Checkstyle violations
(check) on project multi-sourcefolder: You have 7 Checkstyle violations.
After I changed version Checkstyle 8.19 to 8.21 there are other two faled ITs:
multi-sourcefolder: You have 3 Checkstyle violations.
[INFO] multi-modules-aggregate\pom.xml .................. FAILED (8.0 s) [INFO] The post-build script did not succeed. assert content.contains( 'org/apache/maven/plugins/checkstyle/its/App.java' )
@rnveach do you have any idea ? We could have a simple for the failing integration test https://github.com/apache/maven-checkstyle-plugin/pull/16/files/799faa1760d650a6d4f94a70d8014946b4012ef3#diff-b0644a6aeea59931d02c954fc8356903
May I send the path or can you do it @rnveach ? If all integration tests are passing with that simple with I think that we are on our way. How does it sound to you ?
I'm sorry I forgot I was suppose to do something here. I don't have time today to look into it more and make a fix. If you can do something without me for the 2 new ITs, I am fine and then I can rebase this PR to complete it. Otherwise I will try to work on it tomorrow.
@rnveach please go ahead. there is not much hurry, let's to the right things.
Currently maven checkstyle plugin is working very well and users are able to switch to new checkstyle and you (checkstyle maintainers) are able to move forward and drop legacy stuff.
I appreciate very much your effort, I don't have much time during these days
@michael-o This is connected to https://issues.apache.org/jira/browse/MCHECKSTYLE-376 , which is still open. I am not active in this repo right now, so I am going to close this PR.