Issue #32: Main #57

Luolc · 2017-07-20T17:55:24Z

I don't know what to add in pom.xml. I failed to run the project by java -jar ... way.

I haven't handle any exceptions of components now, how should be present the exception message for user? Should we have a [ERROR] prefix or smth like this?

rnveach · 2017-07-20T19:03:14Z

I failed to run the project by java -jar ... way.

I will create the configuration for the all jar.
Edit: you added it, I'll see what is wrong.

how should be present the exception message for user?

I am fine throwing the exception all the way past main.

rnveach

We need an issue on code coverage in POM.

rnveach · 2017-07-20T19:04:17Z

pom.xml

+                                    <transformers>
+                                        <transformer
+                                                implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer">
+                                            <mainClass>com.puppycrawl.tools.checkstyle.Main</mainClass>


Wrong package.

rnveach · 2017-07-20T19:06:18Z

src/main/java/com/github/checkstyle/regression/Main.java

 /**
 * Utility class, contains main function and its auxiliary routines.
 * @author LuoLiangchen
 */
 public final class Main {
+    /** Option name of repository path. */
+    private static final String OPT_REPO = "repo";


Add short names and long names.

https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/diff.groovy#L44
Try to make similar options the same as the groovy scripts, that way there is less a learning curve.
This sound good?

I made this changes:
repo => localGitRepo, and short opt is r
branch => patchBranch and short opt is p

Do we really need a parameter of baseBranch? Is it possible that the base branch is not master? If yes, then git components need to be updated as well.

The option name of checkstyle-tester path is being discussed in the post below.

Short opts are defined in createOptions directly. Need I create static final field instead of writing them in plain text in createOptions?

Do we really need a parameter of baseBranch?

No. I was only recommending changes for existing parameters.

Need I create static final field instead of writing them in plain text in createOptions?

I am fine without this for now.

rnveach · 2017-07-20T19:08:15Z

src/main/java/com/github/checkstyle/regression/Main.java

+    private static final String OPT_BRANCH = "branch";
+
+    /** Option name of checkstyle-tester path. */
+    private static final String OPT_TESTER = "tester";


contribution is the name of the repo. checkstyle-tester is the name of the folder.
Can we choose a better, more precise name?

So checkstyleTester?

rnveach · 2017-07-20T19:10:41Z

src/main/java/com/github/checkstyle/regression/Main.java

+
+        try {
+            cmd = parser.parse(options, args);
+            final String repoPath = cmd.getOptionValue(OPT_REPO);


All paths and other fields need to be validated if they are entered.
Paths must exist and be a directory, etc..
etc..

rnveach · 2017-07-20T19:11:19Z

src/main/java/com/github/checkstyle/regression/Main.java

+                System.out.println("report generated at " + report.getAbsolutePath());
+            }
+        }
+        catch (ParseException ex) {


Let's just throw exception past main.

I think it is OK to throw exceptions from components.

But this is exactly a argument format exception, it is only caused by user input 1. wrong format 2.missing arguments 3. wrong argument number etc. And in this case we need to print help message(by formatter.printHelp) and tell user how to use this tool. So I think this catch is necessary.

wrong format 2.missing arguments 3. wrong argument number etc.

Than why is it covering everything if it is only for user input? What lines are throwing this?

The first line in catch block. I will update this.

Updated. Only the parsing line in try block now.

rnveach · 2017-07-20T19:13:01Z

src/main/java/com/github/checkstyle/regression/Main.java

+        ModuleUtils.setNameToModuleExtractInfo(extractInfos);
+        final List<ModuleInfo> moduleInfos = ModuleCollector.generate(changes);
+        final String configFileName =
+                String.format("config-%s-%d.xml", branch, System.currentTimeMillis());


If we are going to format it, can we do date and time instead of milliseconds?

This is done.

rnveach · 2017-07-20T19:14:10Z

src/main/java/com/github/checkstyle/regression/Main.java

+            final String branch = cmd.getOptionValue(OPT_BRANCH);
+            final boolean stopAfterGenerateConfig = cmd.hasOption(OPT_STOP_AFTER_GENERATE_CONFIG);
+
+            final File config = generateConfig(repoPath, branch);


this and lines below it should all be in runRegression.

What is runRegression? Do you mean create a new method with name runRegression?

rnveach · 2017-07-21T10:14:29Z

src/main/java/com/github/checkstyle/regression/Main.java

+            final String branch = cmd.getOptionValue(OPT_PATCH_BRANCH);
+            final boolean stopAfterGenerateConfig = cmd.hasOption(OPT_STOP_AFTER_GENERATE_CONFIG);
+
+            final File config = generateConfig(repoPath, branch);


What is runRegression? Do you mean create a new method with name runRegression?

yes, and move these liens into it.
I want separation between validating input and running of regression.
Similar to https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L381 and https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L287 .

Done. Add a validateArguments and runRegression

rnveach · 2017-07-21T10:17:08Z

config/import-control.xml

@@ -4,6 +4,9 @@
        "http://www.puppycrawl.com/dtds/import_control_1_1.dtd">

 <import-control pkg="com.github.checkstyle.regression">
+    <allow pkg="org.apache.commons.cli" local-only="true"/>
+    <allow pkg="com.github.checkstyle.regression" local-only="true"/>
+    <allow pkg="java.text"/>


Let's make this local only too for now.

Luolc · 2017-07-23T19:13:39Z

We need an issue on code coverage in POM.

#58

checkstyle-tester is the name of the folder.
Can we choose a better, more precise name?

Now it is named as checkstyleTester. Is that ok?

I forget to reply done to some of the reviews before, sorry.

rnveach · 2017-07-23T20:20:49Z

Now it is named as checkstyleTester. Is that ok?

Shouldn't it end with path to better signify we want the path?

rnveach · 2017-07-23T20:23:01Z

src/main/java/com/github/checkstyle/regression/Main.java

+            throw new IllegalArgumentException(
+                    "path of local git repo must exist and be a directory");
+        }
+        if (!args.stopAfterGenerateConfig() && !existAndIsDirectory(args.testerPath().get())) {


If I don't give any value for testerPath, will args.testerPath() return null?

Optional.get() would throw an exception if we don't have a value. http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/java/util/Optional.java#Optional.get%28%29

I would update this code.

rnveach · 2017-07-23T20:23:44Z

src/main/java/com/github/checkstyle/regression/Main.java

+        final Arguments arguments = ImmutableArguments.builder()
+                .repoPath(cmd.getOptionValue(OPT_LOCAL_GIT_REPO))
+                .branch(cmd.getOptionValue(OPT_PATCH_BRANCH))
+                .testerPath(cmd.getOptionValue(OPT_CHECKSTYLE_TESTER))


please use full name for this one, testerPath. It just seems slightly off.

rnveach · 2017-07-23T20:24:16Z

src/main/java/com/github/checkstyle/regression/Main.java

+        }
+        if (!args.stopAfterGenerateConfig() && !existAndIsDirectory(args.testerPath().get())) {
+            throw new IllegalArgumentException(
+                    "path of checkstyle tester must exist and be a directory");


if I don't give tester path, this statement will look odd as it doesn't also say it is a required field.

rnveach · 2017-07-23T20:24:28Z

src/main/java/com/github/checkstyle/regression/Main.java

+    private static void validateArguments(Arguments args) {
+        if (!existAndIsDirectory(args.repoPath())) {
+            throw new IllegalArgumentException(
+                    "path of local git repo must exist and be a directory");


if I don't give repo path, this statement will look odd as it doesn't also say it is a required field.

rnveach · 2017-07-23T20:25:10Z

src/main/java/com/github/checkstyle/regression/Main.java

+     * @return the generated config file
+     * @throws Exception generation failure
+     */
+    private static File generateConfig(String repoPath, String branch)


move closer to runRegression. it is like a child method.

rnveach · 2017-07-23T20:25:25Z

src/main/java/com/github/checkstyle/regression/Main.java

+
+    /** Represents the CLI arguments. */
+    @Value.Immutable
+    /* default */ interface Arguments {


why the default comment?

Would have a PMD failure without this. Rule:CommentDefaultAccessModifier
https://pmd.github.io/pmd-5.4.0/pmd-java/rules/java/comments.html#CommentDefaultAccessModifier

We need package-private here. private and protected can't be inherited by immutables and public is not necessary.
We have another choice that updating pmd.xml to ignore this, instead of this comment.

ok, ignore this then. we will leave it for now.

rnveach · 2017-07-23T20:26:46Z

src/main/java/com/github/checkstyle/regression/Main.java

+     * @return true if file exists and is a directory
+     */
+    private static boolean existAndIsDirectory(String path) {
+        final File file = new File(path);


if path can be null or empty string, won't this incorrectly return that it exists?
verify first that null or empty string can get into here.

It can't be null, but could be empty if we write smth like path="" in CLI.

Check is added.

Luolc · 2017-07-24T06:48:50Z

@rnveach

Now it is named as checkstyleTester. Is that ok?

Shouldn't it end with path to better signify we want the path?

Update to checkstyleTesterPath

if I don't give repo path, this statement will look odd as it doesn't also say it is a required field.
if I don't give tester path, this statement will look odd as it doesn't also say it is a required field.

As we init the option properties in createOptions, localGitRepo and patchBranch is required, checkstyleTesterPath and stopAfterGenerateConfig is not.
If localGitRepo is missing, then program would stop at

        try {
            cmd = parser.parse(options, args);
        }
        catch (ParseException ex) {
            System.err.println(ex.getMessage());
            formatter.printHelp(null, options);
            System.exit(1);
        }

as there would be a ParseException, with message like Missing required option XX.
Meanwhile, immutable class would validate fields not null when it is being created. So repoPath is impossible to be null in validateArguments method.

But checkstyleTesterPath is not required, so we use Optional field in Arguments. And a NoSuchElementException would be thrown if we try to do checkstyleTesterPath().get() when it is null.
I have updated validateArguments to throw a clearer message for this case.

rnveach

I didn't see anything else.
Please add to issue what was done and what was left when this is merged. Don't forget we need full documentation in the readme.

rnveach · 2017-07-24T12:51:32Z

src/main/java/com/github/checkstyle/regression/Main.java

+    /**
+     * Runs the regression tool.
+     * @param args the parsed CLI arguments.
+     * @throws Exception excute failure


excute => execute

Done. My typo check was accidentally disabled before. :-(

rnveach · 2017-07-24T12:54:40Z

src/main/java/com/github/checkstyle/regression/Main.java

+     * @return the generated config file
+     * @throws Exception generation failure
+     */
+    private static File generateConfig(String repoPath, String branch)


Even though we don't need the entire arguments class, I sorta like sending it better than its pieces. Less documentation and easier to grab new arguments that we add.
What do you think about this?

My mistake. I forgot to update this one.
This is done.

Luolc · 2017-07-24T16:20:50Z

@rnveach

Please add to issue what was done and what was left when this is merged. Don't forget we need full documentation in the readme.

I am facing an error Exception in thread "main" java.lang.SecurityException: Invalid signature file digest for Manifest main attributes, when trying java -jar .. to run the jar file. I have searched about this for a while but failed to solve it. So by now I haven't really use CLI to see how it works now.

Docs in other issue?

rnveach · 2017-07-25T01:53:01Z

Exception in thread "main" java.lang.SecurityException: Invalid signature file digest for Manifest main attributes

I will look into. Please add JAR generation to travis and appveyor.

Docs in other issue?

If you mean adding documentation, it can be in the issue this PR references.

rnveach · 2017-07-25T02:14:56Z

Manifest

Try adding this: https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L927-L947

Luolc · 2017-07-25T17:48:18Z

@rnveach

Try adding this: https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L927-L947
Still having error. pom update is pushed.

My command:

$ mvn clean package -Passembly
$ java -jar target/regression-tool-1.0-SNAPSHOT-all.jar -r path/to/checkstyle -b branch --stopAfterGenerateConfig
Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.SecurityException: Invalid signature file digest for Manifest main attributes
	at sun.security.util.SignatureFileVerifier.processImpl(SignatureFileVerifier.java:314)
	at sun.security.util.SignatureFileVerifier.process(SignatureFileVerifier.java:268)
	at java.util.jar.JarVerifier.processEntry(JarVerifier.java:316)
	at java.util.jar.JarVerifier.update(JarVerifier.java:228)
	at java.util.jar.JarFile.initializeVerifier(JarFile.java:383)
	at java.util.jar.JarFile.getInputStream(JarFile.java:450)
	at sun.misc.URLClassPath$JarLoader$2.getInputStream(URLClassPath.java:940)
	at sun.misc.Resource.cachedInputStream(Resource.java:77)
	at sun.misc.Resource.getByteBuffer(Resource.java:160)
	at java.net.URLClassLoader.defineClass(URLClassLoader.java:454)
	at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:495)

Please add JAR generation to travis and appveyor.

Do you mean adding a mvn clean package -Passembly in travis and appveyor?

rnveach · 2017-07-25T18:19:58Z

Do you mean adding a mvn clean package -Passembly in travis and appveyor?

Yes.
I got a different error when packaging, I will try again soon.

rnveach · 2017-07-25T21:49:56Z

@Luolc see http://zhentao-li.blogspot.com/2012/06/maven-shade-plugin-invalid-signature.html . It is the first result that comes up in google and it fixed the issue for me.

rnveach · 2017-07-25T21:51:46Z

src/main/java/com/github/checkstyle/regression/Main.java

+        }
+        catch (ParseException ex) {
+            System.err.println(ex.getMessage());
+            formatter.printHelp(null, options);


This produces the following exception when called with no arguments:

java.lang.IllegalArgumentException: cmdLineSyntax not provided

This is not helpful to users at all.

Fixed. cmdLineSyntax is not allowed to be null or empty. Now I pass a "regression-tool" to it. (the argument is shown after usage:)
Acts like:

$ java -jar target/regression-tool-1.0-SNAPSHOT-all.jar Missing required options: r, p usage: regression-tool -p,--patchBranch <arg> the name of the PR branch -r,--localGitRepo <arg> the path of the checkstyle repository --stopAfterGenerateConfig indicates that regression tool would stop after generating config -t,--checkstyleTesterPath <arg> the path of the checkstyle-tester directory

Luolc · 2017-07-26T17:20:22Z

Please add JAR generation to travis and appveyor.

Done.

see http://zhentao-li.blogspot.com/2012/06/maven-shade-plugin-invalid-signature.html . It is the first result that comes up in google and it fixed the issue for me.

It works, thanks. I found similar answer at StackOverflow before but it seems missed line <artifact>*:*</artifact>.

rnveach · 2017-07-26T17:41:02Z

usage: regression-tool

It should be java -jar regression-tool.jar (not sure about version number) and shouldn't it include some text describing the optional arguments like other programs do?
Example from java:

Usage: java [-options] class [args...]
          (to execute a class)
  or  java [-options] -jar jarfile [args...]
          (to execute a jar file)

Luolc · 2017-07-27T01:16:13Z

@rnveach
I wrote regression-tool for I read its source code and found the parameter of its internal method is named as appName, while I don't actually know why.

Edit: I find the proper way in javadoc of class HelpFormatter.

formatter.printHelp("myapp", header, options, footer, true);

Sample output:

usage: myapp -f <FILE> [-h] [-v]

Now I understand the meaning of the first parameter. It supposed we are running command like mvn ..., git ... etc. And the last parameter true is turning on autoUsage mode.

Now our code:

formatter.printHelp("java -jar regression-tool.jar", options, true);

Output:

$ java -jar target/regression-tool-1.0-SNAPSHOT-all.jar
Missing required options: r, p
usage: java -jar regression-tool.jar -p <arg> -r <arg>
       [--stopAfterGenerateConfig] [-t <arg>]
 -p,--patchBranch <arg>            the name of the PR branch
 -r,--localGitRepo <arg>           the path of the checkstyle repository
    --stopAfterGenerateConfig      indicates that regression tool would
                                   stop after generating config
 -t,--checkstyleTesterPath <arg>   the path of the checkstyle-tester
                                   directory

One problem is the version number. Should we specify it manually every time we update, or trying to detect the version number from maven settings?

rnveach · 2017-07-27T02:23:26Z

@Luolc lets leave version number to another time. It will have to detect snapshot too. There should be some way for maven to inject the needed data for us to acquire at runtime.

Luolc · 2017-07-27T03:14:31Z

@rnveach OK.
Is there anything else need to improve in this PR?

rnveach · 2017-07-27T11:09:40Z

src/main/java/com/github/checkstyle/regression/Main.java

+        }
+        catch (ParseException ex) {
+            System.err.println(ex.getMessage());
+            formatter.printHelp("regression-tool", options);


waiting for this update...

Sorry I found I didn't successfully push the changes before. Updated.

rnveach

What about this comment at #57 (comment) ?
Edit: fixed link.

Once these 2 are done, I will merge this.

rnveach · 2017-07-27T13:14:39Z

src/main/java/com/github/checkstyle/regression/Main.java

+            }
+            else {
+                throw new IllegalArgumentException("missing checkstyleTesterPath, "
+                        + "which is required if you are using --stopAfterGenerateConfig mode");


shouldn't this be if you are not using --stopAfterGenerateConfig mode?
The if statement above is if (!args.stopAfterGenerateConfig()).

Luolc · 2017-07-27T13:23:04Z

@rnveach

What about this comment at https://github.com/checkstyle/regression-tool/pull/57/files/6a2f57678ca9ae6ab78bb5cd7b5669200be3f45e#discussion_r128928723 ?

I got a We went looking everywhere, but couldn’t find those commits. page.

rnveach · 2017-07-27T13:38:47Z

@Luolc I edited the comment after I noticed it, please re-check the new link.

Luolc · 2017-07-27T13:43:43Z

@rnveach
I mentioned this at #57 (comment)

The explanation in detail is at that post.
If repo path is not given, the program would stop earlier and at that line repo is not possible to be null.

rnveach · 2017-07-27T13:50:26Z

ok, I missed that since it was separate. Once CI is done I will merge.

Luolc · 2017-07-27T13:58:55Z

#59 This issue for creating a document of this tool.

Luolc force-pushed the issue32 branch from 030220e to 54b91ce Compare July 20, 2017 17:56

rnveach requested changes Jul 20, 2017

View reviewed changes

Luolc force-pushed the issue32 branch from 54b91ce to 007edd3 Compare July 21, 2017 02:56

rnveach requested changes Jul 21, 2017

View reviewed changes

Luolc force-pushed the issue32 branch from 007edd3 to 6a2f576 Compare July 23, 2017 19:09

rnveach requested changes Jul 23, 2017

View reviewed changes

Luolc force-pushed the issue32 branch from 6a2f576 to 0de274c Compare July 24, 2017 06:56

rnveach requested changes Jul 24, 2017

View reviewed changes

Luolc force-pushed the issue32 branch from 0de274c to cd2a497 Compare July 24, 2017 16:13

Luolc force-pushed the issue32 branch from cd2a497 to 4edf328 Compare July 25, 2017 17:41

rnveach requested changes Jul 25, 2017

View reviewed changes

Luolc force-pushed the issue32 branch 2 times, most recently from 595c74c to c4ea1a4 Compare July 26, 2017 17:16

rnveach reviewed Jul 27, 2017

View reviewed changes

Luolc force-pushed the issue32 branch from c4ea1a4 to 085d898 Compare July 27, 2017 13:06

rnveach reviewed Jul 27, 2017

View reviewed changes

Issue checkstyle#32: Main

0a07ca2

Luolc force-pushed the issue32 branch from 085d898 to 0a07ca2 Compare July 27, 2017 13:21

rnveach approved these changes Jul 27, 2017

View reviewed changes

rnveach merged commit bb25bd3 into checkstyle:master Jul 27, 2017

Luolc deleted the issue32 branch July 27, 2017 15:00

Issue #32: Main #57

Issue #32: Main #57

Conversation

Luolc commented Jul 20, 2017 • edited by rnveach Loading

rnveach commented Jul 20, 2017 • edited Loading

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 23, 2017

rnveach commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 24, 2017

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 24, 2017 • edited Loading

rnveach commented Jul 25, 2017 • edited Loading

rnveach commented Jul 25, 2017

Luolc commented Jul 25, 2017

rnveach commented Jul 25, 2017 • edited Loading

rnveach commented Jul 25, 2017

rnveach Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc commented Jul 26, 2017

rnveach commented Jul 26, 2017

Luolc commented Jul 27, 2017

rnveach commented Jul 27, 2017

Luolc commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 27, 2017

rnveach commented Jul 27, 2017

Luolc commented Jul 27, 2017

rnveach commented Jul 27, 2017 • edited Loading

Luolc commented Jul 27, 2017

Luolc commented Jul 20, 2017 •

edited by rnveach

Loading

rnveach commented Jul 20, 2017 •

edited

Loading

Luolc Jul 21, 2017 •

edited

Loading

Luolc Jul 21, 2017 •

edited

Loading

Luolc commented Jul 24, 2017 •

edited

Loading

rnveach commented Jul 25, 2017 •

edited

Loading

rnveach commented Jul 25, 2017 •

edited

Loading

rnveach Jul 25, 2017 •

edited

Loading

Luolc Jul 26, 2017 •

edited

Loading

rnveach left a comment •

edited

Loading

rnveach commented Jul 27, 2017 •

edited

Loading