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

Issue #6068: migrate to picocli command line parser from Commons CLI #6126

Merged
merged 1 commit into from
Oct 29, 2018

Conversation

remkop
Copy link
Contributor

@remkop remkop commented Sep 20, 2018

Issue #6068: migrate to picocli command line parser from Commons CLI

This PR replaces #6082.

Sorry it took so long. It was harder than I thought to get all the tests and checks to pass. :-)

@remkop
Copy link
Contributor Author

remkop commented Sep 20, 2018

I may need some help with the failing CI.

The IDEA Inspections Pull Request (Checkstyle) — TeamCity build failed with the below. I don't know
what this means or how to fix it.

[12:13:00]Number of inspection errors 16 is 16 more than the provided threshold 0
[12:13:00]Build failure condition: Fail build if number of inspection errors is more than 0
[12:13:00]Number of inspection warnings 22 is 22 more than the provided threshold 0
[12:13:00]Build failure condition: Fail build if number of inspection warnings is more than 0

ci/circleci: pitest2 — tests failed on CircleCI:

(at first glance this seems a problem in PowerMockJUnit44RunnerDelegateImpl)

12:10:10 PM PIT >> INFO : Created  1 mutation test units
stderr  : 12:11:48 PM PIT >> SEVERE : Error while running adapter JUnit fixture class com.puppycrawl.tools.checkstyle.MainTest with filter Optional[testTreeWalkerThreadsNumber(com.puppycrawl.tools.checkstyle.MainTest)]
java.lang.RuntimeException: PowerMock internal stderr  : error: Should never throw exception at this level
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.handleException(PowerMockJUnit44RunnerDelegateImpl.java:384)
	at org.powermock.modules.junit4.istderr  : nternal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.evaluateStatement(PowerMockJUnit47RunnerDelegateImpl.java:110)
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.execustderr  : teTest(PowerMockJUnit47RunnerDelegateImpl.java:82)
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runBeforesThenTestThenAfters(PowerMockJUnit44RunnerDelegateImpl.java:298)
	at org.junit.internstderr  : al.runners.MethodRoadie.runTest(MethodRoadie.java:87)
	at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:50)
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.invokeTestMethod(PowerMockJUnit44RunnerDelegateImpstderr  : l.java:218)
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.runMethods(PowerMockJUnit44RunnerDelegateImpl.java:160)
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$1.run(PowerMockJUnit44Rustderr  : nnerDelegateImpl.java:134)
	at org.junit.internal.runners.ClassRoadie.runUnprotected(ClassRoadie.java:34)
	at org.junit.internal.runners.ClassRoadie.runProtected(ClassRoadie.java:44)
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelestderr  : gateImpl.run(PowerMockJUnit44RunnerDelegateImpl.java:136)
	at org.powermock.modules.junit4.common.internal.impl.JUnit4TestSuiteChunkerImpl.run(JUnit4TestSuiteChunkerImpl.java:117)
	at org.powermock.modules.junit4.common.internal.impl.AbstractCommonPowerMocstderr  : kRunner.run(AbstractCommonPowerMockRunner.java:57)
	at org.powermock.modules.junit4.PowerMockRunner.run(PowerMockRunner.java:59)
	at org.pitest.junit.adapter.CustomRunnerExecutor.run(CustomRunnerExecutor.java:42)
	at org.pitest.junit.adapter.AdaptedJUnitTestderr  : stUnit.execute(AdaptedJUnitTestUnit.java:69)
	at org.pitest.mutationtest.execute.MutationTimeoutDecorator.lambda$createRunnable$0(MutationTimeoutDecorator.java:83)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.costderr  : ncurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ArrayIndexOutOfBoundsException: 0
	at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.hanstderr  : dleException(PowerMockJUnit44RunnerDelegateImpl.java:362)
	... 20 more

@rnveach
Copy link
Member

rnveach commented Sep 20, 2018

IDEA Inspections Pull Request

You have to view the report at https://teamcity.jetbrains.com/viewLog.html?buildId=1628574&tab=Inspection&buildTypeId=Checkstyle_IdeaInspectionsPullRequest .
When you first enter the CI and sign in for anonymous access, there is a link that says view report.

pitest2

I am not sure if that exception is related. Usually the CI will tell you what is missing, but it looks like it failed to do that for some reason.
@romani Please look.

Mutation score of 99 is below threshold of 100

You may have to run pitest locally and look at the report to see what is missing.

@remkop remkop force-pushed the #6068-v2-picocli-cli-parser branch 20 times, most recently from c378899 to cfda81c Compare September 25, 2018 15:56
@remkop
Copy link
Contributor Author

remkop commented Sep 26, 2018

@rnveach, @romani, Please consider this PR. Sorry for the delay. I found it quite difficult to get the CI checks to pass (I’m relieved they pass now; I was close to giving up a few times). :-)

This PR migrates the Main and JavadocPropertiesGenerator command line utilities from Commons CLI to picocli.

Note that I used picocli’s mixinStandardHelpOptions function to provide --help and --version options without the need for an annotated field. I hope you agree that this is useful. I updated the documentation accordingly.

@rnveach, I considered moving the annotated fields into an inner class but I found the current code to be cleaner: we would end up either moving a significant portion of the logic into the inner class, or alternatively referencing the option fields indirectly like this:

if (options.printAst || options.printAstWithComments
        || options.printJavadocTree || options.printTreeWithJavadoc) {
    if (options.suppressionLineColumnNumber != null 
            || options.configurationFile != null 
            || options.propertiesFile != null 
            || options.outputPath != null
...

The current code (without the options reference) seems simpler and cleaner. I hope you agree.

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remkop It is alot to review and scanned some of it.

I question the use of so many no-inspections but that would require looking into this more.

config/spotbugs-exclude.xml Outdated Show resolved Hide resolved
config/intellij-idea-inspections.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/Main.java Outdated Show resolved Hide resolved
@remkop
Copy link
Contributor Author

remkop commented Sep 27, 2018

@rnveach Fixed some of the things you pointed out. I amended the commit with force-push to keep history clean. Is that the preferred way or do you prefer additional commits in response to feedback?

@remkop remkop force-pushed the #6068-v2-picocli-cli-parser branch 2 times, most recently from 412faaf to 29c05ef Compare September 27, 2018 12:19
@remkop
Copy link
Contributor Author

remkop commented Oct 23, 2018

@romani Thanks for the detailed review. All items you mentioned have been addressed. CI build is green. Please take another look.

Out of interest, what time zone are you in? I’m in Tokyo (JST).

@romani
Copy link
Member

romani commented Oct 27, 2018

I am in PST

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now code is really good and modular, far better than our previous cli version.

the very last items:

@romani
Copy link
Member

romani commented Oct 27, 2018

@rnveach , please start code review.

Here is how it works:

$ java -jar checkstyle-8.15-SNAPSHOT-all.jar
Missing required parameter: <files>
Usage: checkstyle [-dhjJtTV] [--executeIgnoredModules] [-gxs] [-c=<configurationFile>]
                  [-C=<checkerThreadsNumber>] [-f=<format>] [-o=<outputPath>] [-p=<propertiesFile>]
                  [-s=<suppressionLineColumnNumber>] [-tabWidth=<tabWidth>]
                  [-W=<treeWalkerThreadsNumber>] [-e=<exclude>]... [-x=<excludeRegex>]... <files>...
Checkstyle verifies that the specified source code files adhere to the specified rules. By default
errors are reported to standard out in plain format. Checkstyle requires a configuration XML file
that configures the checks to apply.
      <files>...             One or more source files to verify
      --executeIgnoredModules
                             Allows ignored modules to be run.
  -c= <configurationFile>    Sets the check configuration file to use.
  -C, --checker-threads-number=<checkerThreadsNumber>
                             (experimental) The number of Checker threads (must be greater than zero)
  -d, --debug                Print all debug logging of CheckStyle utility
  -e, --exclude=<exclude>    Directory path to exclude from CheckStyle
  -f= <format>               Sets the output format. Valid values: xml, plain. Defaults to plain
      -gxs, --generate-xpath-suppression
                             Generates to output a suppression.xml to use to suppress all violations
                               from user's config
  -h, --help                 Show this help message and exit.
  -j, --javadocTree          Print Parse tree of the Javadoc comment
  -J, --treeWithJavadoc      Print full Abstract Syntax Tree of the file
  -o= <outputPath>           Sets the output file. Defaults to stdout
  -p= <propertiesFile>       Loads the properties file
  -s= <suppressionLineColumnNumber>
                             Print xpath suppressions at the file's line and column position. Argument
                               is the line and column number (separated by a : ) in the file that the
                               suppression should be generated for
  -t, --tree                 Print Abstract Syntax Tree(AST) of the file
  -T, --treeWithComments     Print Abstract Syntax Tree(AST) of the file including comments
      -tabWidth=<tabWidth>   Sets the length of the tab character. Used only with "-s" option. Default
                               value is 8
  -V, --version              Print version information and exit.
  -W, --tree-walker-threads-number=<treeWalkerThreadsNumber>
                             (experimental) The number of TreeWalker threads (must be greater than zero)
  -x, --exclude-regexp=<excludeRegex>
                             Regular expression of directory to exclude from CheckStyle

nuances:

  1. Usage is printed if any problems with agreements, I am not sure it very good approach, usually cli tools just print error.
  2. probably extra empty line is required between error message and usage if we want to keep usage printing.
  3. "-tabWidth" , it has only one "-", @remkop it looks like it was our mistake :( and there should be "--", please fix it.
  4. "-gxs" looks weird now, it was like this even before, @remkop , does it make sense to make it "-g" ?
  5. -c= <configurationFile> Sets the check configuration file to use. - there is space after = please explain meaning of it.
    Example:
$ java -jar checkstyle-8.15-SNAPSHOT-all.jar -c 1.xml ../pom.xml 
Could not find config XML file '1.xml'.
$ java -jar checkstyle-8.15-SNAPSHOT-all.jar -c=1.xml ../pom.xml 
Could not find config XML file '1.xml'.
$ java -jar checkstyle-8.15-SNAPSHOT-all.jar -c= 1.xml ../pom.xml 
com.puppycrawl.tools.checkstyle.api.CheckstyleException: unable to parse configuration stream - Premature end of file.:1:1

vs -tabWidth=<tabWidth> with no space

$ java -jar checkstyle-8.15-SNAPSHOT-all.jar -c 1.xml -tabWidth=3 ../pom.xml 
Could not find config XML file '1.xml'.
07:00 $ java -jar checkstyle-8.15-SNAPSHOT-all.jar -c 1.xml -tabWidth 3 ../pom.xml 
Could not find config XML file '1.xml'.

@remkop , please share best practices, you probably saw a lot of CLI helps, your proposals are valuable.

@remkop remkop force-pushed the #6068-v2-picocli-cli-parser branch from ea98b56 to a714eec Compare October 27, 2018 23:10
@remkop
Copy link
Contributor Author

remkop commented Oct 27, 2018

  1. Yes, for a long usage help message like CheckStyle's it may make sense to just show an error message after the user specified invalid input and inform the user they can get detailed help with checkstyle --help. Future versions of picocli will do this automatically for applications that use the CommandLine::run and CommandLine::call convenience methods.
  2. I changed -tabWidth to --tabWidth with two leading hyphens
  3. I agree that having -g looks better in the usage help. UPDATE: I went ahead and replaced -gxs with -g altogether. (was: I added -g, and kept -gxs to avoid breaking existing 3rd party tools that are using this option. Let me know if you want to remove -gxs altogether.)
  4. This is a display issue in the picocli usage help message table layout for short options. I raised Usage help should not show space between short option name and parameter remkop/picocli#531 for this. Is this a showstopper, or is the PR overall still better than the current Commons CLI version?

@remkop remkop force-pushed the #6068-v2-picocli-cli-parser branch 4 times, most recently from 866d096 to 14b6a1e Compare October 28, 2018 00:21
@romani
Copy link
Member

romani commented Oct 28, 2018

Let me know if you want to remove -gxs altogether.

Please remove, our cli is not used that much, this parameter is new, feature behind him is not done up to the end. Thanks a lot for your help.

Yes, for a long usage help message like CheckStyle's it may make sense to just show an error message after the user specified invalid input

Can library be configured now to not print whole help ? If yes, please do. If not - please create an issue on picocli and issue on us, to not forget to fix it. It is annoying but not a showstopper.

Is this a showstopper, or is the PR overall still better than the current Commons CLI version?

Please create separate issue on us for this (to not forget ). Not a show stopper.


As you modify code, please do me a favour and move listFiles right after getFilesToProcess, please see https://methods-distance.herokuapp.com/dsm?source_url=https://raw.githubusercontent.com/checkstyle/checkstyle/14b6a1e1796b8e9902f20137506dbc2f7a75de4e/src/main/java/com/puppycrawl/tools/checkstyle/Main.java , in ideal case red cells should be very close to diagonal.

@remkop
Copy link
Contributor Author

remkop commented Oct 28, 2018

All done:

  • -gxs removed
  • Checkstyle now prints the error message followed by a short usage message and a hint to use checkstyle --help for more information after the user specified invalid input. The full usage help message is only printed on user request.
  • I created issue upgrade to latest picocli version to fix extra space in help output #6175 as a reminder for the display issue in the usage help (the spacing issue). Glad this is not a showstopper. :-)
  • I moved listFiles to after getFilesToProcess, and also some other methods so they can be read more fluently

@remkop remkop force-pushed the #6068-v2-picocli-cli-parser branch 3 times, most recently from 88f80ca to 1670edd Compare October 29, 2018 09:01
@remkop
Copy link
Contributor Author

remkop commented Oct 29, 2018

I had to resubmit a few times to get past transient CI build failures, but all checks passed now.

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I will see much else.

@@ -40,8 +40,8 @@ java -D&lt;property&gt;=&lt;value&gt; \
-c &lt;configurationFile&gt; \
[-f &lt;format&gt;] [-p &lt;propertiesFile&gt;] [-o &lt;file&gt;] \
[-s &lt;line:column&gt;] [-gxs | --generate-xpath-suppression] [-tabWidth &lt;length&gt;] \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gxs needs to be removed here and down below.

Please verify all commands are correct and no others have been changed/added/removed.
Please tell us when both are done individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find. Both done:

  • -gxs replaced by -g in synopsis as well as the option description below
  • verified other options and made two updates to the documentation:
    • -tabWidth became --tabWidth as agreed
    • -executeIgnoredModules became --executeIgnoredModules as agreed

</rule>
<!-- Some fields with picocli annotations will be only used by
the picocli and not by the application.
They may appear unused by static code analysis tools. -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this comment to right above the <property tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I updated the PR. Waiting for the CI pipeline to pass now.

private static final String USAGE = String.format(Locale.ROOT,
"usage: java com.puppycrawl.tools.checkstyle.Main [options] -c <config.xml>"
Copy link
Member

@rnveach rnveach Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romani Do we need to create a section documenting how some arguments changed between the versions?
See my next comment on why I am asking.

@remkop Some other utilities I have created use the CLI as an api to execute checkstyle.
I know some arguments changed, but I don't know the extent. If any of these are affected, please let me know.
https://github.com/checkstyle/contribution/blob/master/checkstyle-web-tester/checkstyle.php#L136
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/launch_diff.sh#L208
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/launch_diff_antlr.sh#L180
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/sevntu_launch_diff.sh#L208
They are mostly -J, -c, -f, -o and return code -2/-1.

Copy link
Member

@romani romani Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to create a section documenting how some arguments changed between the versions?

we can make comment in issue, to reference to it if somebody ask a questions. As far as know we change only "-gxs" (renamed to "-g") and "-V" for version, all other stay the same. Minor changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this comment that summarizes the option name changes: #6068 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rnveach I checked the 4 links above and I believe none of them should be impacted by the changes in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remkop Thanks.

@romani
Copy link
Member

romani commented Oct 29, 2018

@remkop ,

Yes, for a long usage help message like CheckStyle's it may make sense to just show an error message after the user specified invalid input and inform the user they can get detailed help with checkstyle --help. Future versions of picocli will do this automatically for applications that use the CommandLine::run and CommandLine::call convenience methods.

This is still an issue on our side, we are not going to use such convenience methods, they are against our design ideas. I do believe we will not be alone to about extra "extends" and do more compositions.
How picocli can support our use case ?
If that is not possible please create issue in you and our side, users will quickly notice this change, I doubt they will be very happy, it is unusual behavior for cli tools.

@remkop
Copy link
Contributor Author

remkop commented Oct 29, 2018

@romani I think there is a misunderstanding: in my last update for the PR I already implemented your request: Checkstyle now prints the error message followed by a short usage message and a hint to use checkstyle --help for more information after the user specified invalid input. The full usage help message is only printed on user request.

Please see lines 126 and 127 in Main.

I believe this is exactly what you asked for. Did I misunderstand?

Please disregard my earlier comment on my plans for supporting this in future versions of picocli. Since Checkstyle does not use the convenience methods, it will not be impacted by such future changes.

@remkop remkop force-pushed the #6068-v2-picocli-cli-parser branch from 1670edd to 7c5a1c6 Compare October 29, 2018 21:51
@romani
Copy link
Member

romani commented Oct 29, 2018

I believe this is exactly what you asked for. Did I misunderstand?

my bad, I did not noticed this change. Here is how it works in comparison with "grep":

$ java -jar checkstyle-8.15-SNAPSHOT-all.jar -c 1.xml -tabWidth 3 ../pom.xml 
Unknown option: -abWidth
Usage: checkstyle [OPTIONS]... FILES...
Try 'checkstyle --help' for more information.

$ grep -TTT
Usage: grep [OPTION]... PATTERN [FILE]...
Try 'grep --help' for more information.

looks good.
I am done with review. PR is approved.
@remkop , thanks a lot for your hard work.

@remkop
Copy link
Contributor Author

remkop commented Oct 29, 2018

I’m hoping that other people will follow CheckStyle’s example and adopt picocli in their project.

If you like what you’ve seen so far, please say some nice things about picocli in a few public places. It really helps!

@romani
Copy link
Member

romani commented Oct 29, 2018

I will twit about it, ones we merge your PR.

Travis was restarted, it was failed for unrelated issues.

Teamcity is complaining due to time out (30 min is limit). Here is history of builds:

27 Oct 18 05:22 25m:45s
26 Oct 18 04:23 23m:55s
26 Oct 18 00:41 25m:31s
23 Oct 18 20:38 24m:00s
21 Oct 18 18:14 26m:43s
20 Oct 18 22:24 24m:00s
20 Oct 18 20:47 25m:29s
20 Oct 18 20:18 14m:28s
20 Oct 18 19:58 26m:25s
20 Oct 18 17:41 27m:46s
20 Oct 18 17:23 26m:47s
16 Oct 18 13:53 30m:07s
16 Oct 18 13:31 26m:36s
16 Oct 18 00:53 24m:49s
14 Oct 18 15:28 25m:11s
7 Oct 18 18:09 21m:48s
6 Oct 18 15:02 21m:24s
4 Oct 18 14:50 19m:16s

Average 22m.

in PRs, average is 27

29 Oct 18 22:27 27m:51s
29 Oct 18 21:52 35m:05s
29 Oct 18 16:34 27m:56s
29 Oct 18 15:59 35m:08s
29 Oct 18 15:40 26m:59s
29 Oct 18 12:59 32m:48s
29 Oct 18 12:11 29m:33s
29 Oct 18 11:51 30m:20s
29 Oct 18 11:48 28m:25s
29 Oct 18 09:03 27m:14s
29 Oct 18 05:00 27m:01s
29 Oct 18 03:44 26m:04s
28 Oct 18 22:14 27m:06s
28 Oct 18 17:30 27m:25s
28 Oct 18 17:07 25m:33s
28 Oct 18 00:24 27m:46s
27 Oct 18 23:16 28m:15s
27 Oct 18 23:10 26m:43s
27 Oct 18 22:57 28m:39s
27 Oct 18 05:23 28m:27s
27 Oct 18 04:26 26m:08s
27 Oct 18 04:14 25m:40s
27 Oct 18 03:31 26m:47s
26 Oct 18 22:05 35m:07s
26 Oct 18 04:29 25m:04s
26 Oct 18 00:41 26m:00s
26 Oct 18 00:40 28m:00s
26 Oct 18 00:38 28m:19s
24 Oct 18 08:37 31m:52s
24 Oct 18 08:29 30m:44s
24 Oct 18 07:13 27m:22s

We are running to close to limits.

Master build limit is set to 35 min.
PR build limit was set to 40 min.

@romani romani merged commit f3143fa into checkstyle:master Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants