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

Add build instructions to README #6236

Closed
krichter722 opened this Issue Dec 1, 2018 · 15 comments

Comments

Projects
None yet
3 participants
@krichter722
Copy link
Contributor

krichter722 commented Dec 1, 2018

There're no build instructions in README, not is there a CONTRIBUTING or INSTALL in the source root. The instructions should be mentioned even if they're the expected default in order to make sure they're supported. Build information should be available in at least one expected location either included or linked (including links to external/online documentation).

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 1, 2018

Please suggest update to readme file,
We have https://checkstyle.org/contributing.html
https://github.com/checkstyle/checkstyle/wiki/Build-all-jars-and-generate-maven-site

Please let us know what you tried to build.

@romani romani added the approved label Dec 1, 2018

@krichter722

This comment has been minimized.

Copy link
Contributor Author

krichter722 commented Dec 1, 2018

Please suggest update to readme file

Will do.

Please let us know what you tried to build.

mvn install fails in a fresh clone because of a few thousand checkstyle errors. I guess that what a number of people would use to build the project:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:1.8:run (ant-phase-verify) on project checkstyle: An Ant BuildException has occured: The following error occurred while executing this line:
[ERROR] /tmp/checkstyle/config/ant-phase-verify.xml:162: Checkstyle failed: Got 3845 errors and 0 warnings.
[ERROR] around Ant part ...<ant antfile="config/ant-phase-verify.xml"/>... @ 8:47 in /tmp/checkstyle/target/antrun/build-main.xml

I can provide more details in a separate issue. Afaik this doesn't happen on Travis CI. I use maven 3.5 on Ubuntu 18.10 locally.

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 1, 2018

And now it become interesting, please focus on details on how you was able to fail an "mvn install", it supposed to be working on all OS and all locales. We even have CI to keep this under control.

Let's focus on details of your OS. Please provide full log "mvn -X ...".

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 2, 2018

I guess that what a number of people would use to build the project

mvn install - will generate maven standard jar
mvn package -P assembly - will generate uber jar
if you know how to update our code to generate uber jar without special profile - feel free to make an issue on this and provide PR.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Dec 3, 2018

@romani

please focus on details on how you was able to fail an "mvn install",

#6235 (comment)

looks like you from Germany, so you might have special locale n your linux

This is the reason for his failure. When we added message suppression, CI and maven default to your current locale. Our CI is set to english so our suppression files have messages in english.

Checkstyle failed: Got 3845 errors and 0 warnings.

I confirmed locally, and this is the same number of errors I get when I remove our suppressions by message.

This is a known issue, see: #5350

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 3, 2018

@krichter722 , please confirm that you have the same issue, looks like we need to extend our CI to check checkstyle execution on non-english locale. It is really bad experience to have no ability to simply build project out of the box.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Dec 28, 2018

@krichter722

This comment has been minimized.

Copy link
Contributor Author

krichter722 commented Dec 28, 2018

Please find attached the output of mvn -X clean install in a fresh clone on my Ubuntu 18.10 with German language pack installed and in use.
mvn.log

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 28, 2018

@krichter722 , thanks a lot.

> mvn -X clean install
OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=512m; support was removed in 8.0
Apache Maven 3.5.4
Maven home: /usr/share/maven
Java version: 1.8.0_191, vendor: Oracle Corporation, runtime: /usr/lib/jvm/java-8-openjdk-amd64/jre
Default locale: de_DE, platform encoding: UTF-8
OS name: "linux", version: "4.18.0-13-generic", arch: "amd64", family: "unix"
....
Resource com/puppycrawl/tools/checkstyle/checks/coding/messages.properties loaded from ant loader
Resource com/puppycrawl/tools/checkstyle/checks/coding/messages_de.properties loaded from ant loader
Couldn't load Resource com/puppycrawl/tools/checkstyle/checks/coding/messages_de_DE.properties
Couldn't load Resource com/puppycrawl/tools/checkstyle/checks/coding/messages_de_DE.properties
[checkstyle] [ERROR] /tmp/checkstyle/src/it/java/com/google/checkstyle/test/base/AbstractIndentationTestSupport.java:1: Es fehlt eine package-info.java. [JavadocPackage]

it is clear problem with not working suppression, all violations are in src/test and src/it:

$ cat mvn.log | grep "\[checkstyle\] \[ERROR\]" | cut -d ' ' -f 3 | sed "s/\/tmp\/checkstyle\///" | grep -vE "src/(test|it)/java"
✘-1

problems with javadoc suppressions:

$ cat mvn.log | grep "\[checkstyle\] \[ERROR\]" | sed -E "s/.*\[//" | sed "s/\]//" | sort | uniq
JavadocMethod
JavadocPackage
JavadocType
JavadocVariable

suppression in english:
https://github.com/checkstyle/checkstyle/blob/master/config/suppressions.xml#L33
We need to make suppression by Checks names, to make our suppressions international.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Dec 28, 2018

This is all a duplicate of #5350

We need to make suppression by Checks names

Unless we split the checks apart, this isn't possible. These checks print multiple different violation messages, and suppression is only suppressing 1 message and not the others.

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 30, 2018

closed as duplicate of #5350

@romani romani closed this Dec 30, 2018

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 31, 2018

@krichter722 , we merged fix for not working suppression.
Please try to build on your local one more time.
Thanks a lot for your feedback.

@krichter722

This comment has been minimized.

Copy link
Contributor Author

krichter722 commented Dec 31, 2018

@romani checkstyle-8.16-4-g89c8642bc builds successfully with mvn clean install a the implicit default german locale. Thanks for the fix. I'd still be happy if the build instructions were mentioned or linked in README although they're the default.

@romani romani reopened this Dec 31, 2018

@romani

This comment has been minimized.

Copy link
Member

romani commented Dec 31, 2018

Make sense , new chapter should be before Continuous integration and Quality reports with links to html pages

romani added a commit that referenced this issue Feb 3, 2019

romani added a commit that referenced this issue Feb 4, 2019

@romani romani added the miscellaneous label Feb 4, 2019

@romani romani added this to the 8.18 milestone Feb 4, 2019

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 4, 2019

fix is merged.

@romani romani closed this Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.