[MCHECKSTYLE-376] Remove window line endings from files #16

rnveach · 2019-05-25T12:40:14Z

mvn verify and mvn -Prun-its clean verify passed.

Found these doing find . -type f -name '*.*' -exec dos2unix '{}' + on linux.

eolivelli · 2019-05-25T15:15:52Z

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

eolivelli · 2019-05-25T15:19:36Z

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

eolivelli · 2019-05-25T15:41:17Z

Will Windows users (developers of the plugin I mean) be happy with this change? Or is it better to have CRLF every where?

jjlharrison · 2019-05-25T15:54:46Z

Windows users should be fine thanks to Git autocrlf.

eolivelli · 2019-05-25T17:39:05Z

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/

rnveach · 2019-05-25T18:44:56Z

@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.

jjlharrison · 2019-05-25T19:43:46Z

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.

Tibor17 · 2019-05-25T20:17:31Z

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.

eolivelli · 2019-05-25T20:44:27Z

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.

Tibor17 · 2019-05-25T21:03:53Z

@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 · 2019-05-25T21:25:22Z

@Tibor17 which fix in checkstyle?

Tibor17 · 2019-05-25T21:44:01Z

@eolivelli
I mean checkstyle/checkstyle#6559
One commit has been already pushed checkstyle/checkstyle@d2a53a9 but the checkstyle team can say more.

eolivelli · 2019-05-25T21:54:15Z

@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

rnveach · 2019-05-25T22:49:34Z

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.

rnveach · 2019-05-25T22:59:43Z

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.

rnveach · 2019-05-25T23:25:31Z

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.

eolivelli

Very good.
Let me do the bureaucratic part (push to ASF repo and wait for CI)
we will soon have the repository back in great shape, ready for new enhancements.

Thanks

eolivelli · 2019-05-26T07:09:22Z

src/it/MCHECKSTYLE-54/checkstyle.xml

+<!DOCTYPE module PUBLIC "-//Checkstyle//DTD Checkstyle Configuration 1.2//EN" "https://checkstyle.org/dtds/configuration_1_2.dtd">
+<module name="Checker">
+  <module name="TreeWalker">
+    <module name="JavadocMethod"/>


eolivelli · 2019-05-30T16:36:07Z

@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

eolivelli · 2019-05-30T16:49:23Z

@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 ?

eolivelli · 2019-05-30T17:55:33Z

The build is failing on every windows machine.
I am trying to update checkstyle dep now to latest and greatest

Tibor17 · 2019-05-30T17:59:40Z

@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?

rnveach · 2019-05-30T19:56:18Z

[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.

Tibor17 · 2019-05-30T19:58:00Z

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' )

eolivelli · 2019-06-04T12:41:32Z

@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 ?

rnveach · 2019-06-04T14:37:03Z

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.

eolivelli · 2019-06-04T15:44:57Z

@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

Closes #16

bmarwell · 2021-01-23T08:58:15Z

src/it/MCHECKSTYLE-225-pluginManagement/invoker.properties

+# specific language governing permissions and limitations
+# under the License.
+
+invoker.goals = verify
 invoker.debug = true


I think you removed the newline at the end of this file.

Closes #16

romani mentioned this pull request May 25, 2019

Issue #6558: make tests work reliably on Windows with LF line endings checkstyle/checkstyle#6559

Merged

[MCHECKSTYLE-376] - Remove window line endings from files and config 54

799faa1

rnveach force-pushed the windows_line_endings branch from 157c1e7 to 799faa1 Compare May 25, 2019 23:22

eolivelli approved these changes May 26, 2019

View reviewed changes

eolivelli changed the title ~~Remove window line endings from files~~ MCHECKSTYLE-376 Remove window line endings from files Jun 4, 2019

elharo changed the title ~~MCHECKSTYLE-376 Remove window line endings from files~~ [MCHECKSTYLE-376] Remove window line endings from files Aug 13, 2020

asfgit pushed a commit that referenced this pull request Dec 20, 2020

[MCHECKSTYLE-376] - Remove windows line endings from files and config

fdda8e1

Closes #16

bmarwell reviewed Jan 23, 2021

View reviewed changes

slachiewicz pushed a commit that referenced this pull request Sep 5, 2021

[MCHECKSTYLE-376] - Remove windows line endings from files and config

bcff5a6

Closes #16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MCHECKSTYLE-376] Remove window line endings from files #16

[MCHECKSTYLE-376] Remove window line endings from files #16

rnveach commented May 25, 2019 •

edited

eolivelli commented May 25, 2019

eolivelli commented May 25, 2019

eolivelli commented May 25, 2019

jjlharrison commented May 25, 2019

eolivelli commented May 25, 2019

rnveach commented May 25, 2019 •

edited

jjlharrison commented May 25, 2019

Tibor17 commented May 25, 2019

eolivelli commented May 25, 2019

Tibor17 commented May 25, 2019

eolivelli commented May 25, 2019

Tibor17 commented May 25, 2019

eolivelli commented May 25, 2019

rnveach commented May 25, 2019

rnveach commented May 25, 2019 •

edited

rnveach commented May 25, 2019

eolivelli left a comment

eolivelli May 26, 2019

eolivelli commented May 30, 2019

eolivelli commented May 30, 2019

eolivelli commented May 30, 2019

Tibor17 commented May 30, 2019

rnveach commented May 30, 2019

Tibor17 commented May 30, 2019 •

edited

eolivelli commented Jun 4, 2019

rnveach commented Jun 4, 2019

eolivelli commented Jun 4, 2019

bmarwell Jan 23, 2021

[MCHECKSTYLE-376] Remove window line endings from files #16

Are you sure you want to change the base?

[MCHECKSTYLE-376] Remove window line endings from files #16

Conversation

rnveach commented May 25, 2019 • edited

eolivelli commented May 25, 2019

eolivelli commented May 25, 2019

eolivelli commented May 25, 2019

jjlharrison commented May 25, 2019

eolivelli commented May 25, 2019

rnveach commented May 25, 2019 • edited

jjlharrison commented May 25, 2019

Tibor17 commented May 25, 2019

eolivelli commented May 25, 2019

Tibor17 commented May 25, 2019

eolivelli commented May 25, 2019

Tibor17 commented May 25, 2019

eolivelli commented May 25, 2019

rnveach commented May 25, 2019

rnveach commented May 25, 2019 • edited

rnveach commented May 25, 2019

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli May 26, 2019

Choose a reason for hiding this comment

eolivelli commented May 30, 2019

eolivelli commented May 30, 2019

eolivelli commented May 30, 2019

Tibor17 commented May 30, 2019

rnveach commented May 30, 2019

Tibor17 commented May 30, 2019 • edited

eolivelli commented Jun 4, 2019

rnveach commented Jun 4, 2019

eolivelli commented Jun 4, 2019

bmarwell Jan 23, 2021

Choose a reason for hiding this comment

rnveach commented May 25, 2019 •

edited

rnveach commented May 25, 2019 •

edited

rnveach commented May 25, 2019 •

edited

Tibor17 commented May 30, 2019 •

edited