Clean up CLI #448

basil · 2023-02-07T22:51:55Z

This PR cleans up the command-line interface (CLI) of this component, probably not to a final state but to a state significantly better than the status quo which allows a lot of upstream code to be deleted and allows one bug to be fixed.

As part of this PR I have swapped out JCommander for the more elegant picocli. One of the things this allows us to do is easily accumulate properties in a map with multiple -D arguments. Apart from switching to picocli's casing and dash convention, I preserved existing semantics but with one important exception: I redefined the passing of Maven properties to apply only to Maven test execution, not Maven compilation. This is an important change in semantics that allows for a lot of cleanup to be done. It results in no loss of functionality, because the existing "Maven arguments" functionality can be used to pass properties to compilation. But allowing properties to be passed solely for test execution turns out to be quite handy. This leaves us with the following Maven-related properties:

--mvn: Global path to the Maven executable, passed to both compilation and tests
--m2-settings: Global settings file, passed to both compilation and tests
--maven-args: Global Maven arguments, passed to both compilation and tests (currently unused)
-D: Passed just when running tests

With this functionality there was simply no need to allow for properties to be passed in via a file anymore, so I ripped that out. (The only reason before was to represent properties that had a colon, but that is now supported natively with the picocli -D syntax.)

With that out of the way, I went ahead and transferred the responsibility for setting overrideWar, jenkins.version, and useUpperBounds from consumers of this component to this component itself. This simplifies consumers and de-duplicates code. And of course it is only passed when running tests, so compilation takes place with as vanilla a configuration as possible to achieve the originally intended goal of testing binary compatibility.

This PR also removes any references to -DforkCount or -Dsurefire.excludesFile from this component, leaving the responsibility for setting these firmly in the hands of consumers. Both consumers already set them, and this seems like the right place to do it. So no need to set them in this component.

There is still some messiness that remains duplicated in consumers after this PR: setting -Denforcer.skip, -Dhpi-plugin.version, -Djth.jenkins-war.path, and -DoverrideWarAdditions. These can be cleaned up separately, but at least they now only apply to tests, which is a strict improvement over the status quo.

Testing done

Besides jenkinsci/bom#1735 I manually tested text-finder with both old and new versions of Jenkins core and verified that the Maven properties were set correctly in each case (for both compilation and testing).

How to adapt consumers

See jenkinsci/bom#1735 for an example.

Replace -war with --war
Replace -workDirectory with --working-dir
Replace -includePlugins with --include-plugins
Replace -excludePlugins with --exclude-plugins
Replace -excludeHooks with --exclude-hooks
Replace -fallbackGitHubOrganization with --fallback-github-organization
Replace -m2SettingsFile with --m2-settings
Replace -mavenProperties with -Dkey=value for each property. Use multiple -D arguments, one for each property. (These will only apply to test execution, not compilation, which is probably what you want anyway.)
Replace -mavenPropertiesFile with -Dkey=value for each property. Use multiple -D arguments, one for each property. (These will only apply to test execution, not compilation, which is probably what you want anyway.)
Replace -mavenArgs with --maven-args
Replace -hookPrefixes with --hook-prefixes
Replace -externalHooksJars with --external-hooks-jars
Replace -localCheckoutDir with --local-checkout-dir
Replace -failFast true with --fail-fast
Replace -failFast false with --no-fail-fast
If you were passing overrideWar as a Maven property, stop doing that. We'll now add it automatically when running tests.
If you were passing jenkins.version as a Maven property, stop doing that. We'll now add it automatically when running tests.
If you were passing useUpperBounds as a Maven property, stop doing that. We'll now add it automatically when running tests.
If you were passing upperBoundsExcludes as a Maven property, stop doing that. We'll now add it automatically when running tests, but only if required for older Jenkins cores.
Replace calls to new ExternalMavenRunner(config.getExternalMaven()) with new ExternalMavenRunner(config.getExternalMaven(), config.getM2Settings(), config.getMavenArgs())`
Replace the first argument to MavenRunner#run with a map of properties. Assuming your usage is calling Maven for compilation rather than testing, this should just be Map.of("maven.javadoc.skip", "true"). Finally move any properties that you might have in the old MavenRunner.Config mconfig argument into this map. For example the first argument would read Map.of("maven-hpi-plugin.disabledTestInjection", "true", "maven.javadoc.skip", "true") followed by deleting the MavenRunner.Config mconfig variable.

basil · 2023-02-08T01:57:00Z

pom.xml

+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <!-- Version specified in parent POM -->
+        <configuration>
+          <annotationProcessorPaths>
+            <path>
+              <groupId>info.picocli</groupId>
+              <artifactId>picocli-codegen</artifactId>
+              <version>${picocli.version}</version>
+            </path>
+          </annotationProcessorPaths>
+          <compilerArgs>
+            <arg>-Aproject=${project.groupId}/${project.artifactId}</arg>
+          </compilerArgs>
+        </configuration>
+      </plugin>


This is optional, but including this annotation processor gives feedback faster at compile-time rather than at runtime if an invalid CLI option is present.

basil · 2023-02-08T01:58:07Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

+        runner =
+                new ExternalMavenRunner(
+                        config.getExternalMaven(), config.getM2Settings(), config.getMavenArgs());


The new calling convention for ExternalMavenRunner sets up the path to Maven and "global" arguments (including the settings file) but not properties: these are now specific to the type of Maven run (compilation or testing) and are passed in to MavenRunner#run on a case-by-case basis.

basil · 2023-02-08T01:58:37Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

-        MavenRunner.Config mconfig = new MavenRunner.Config(config);
-        // TODO REMOVE
-        mconfig.userProperties.put("failIfNoTests", "false");
-


This was unneeded; a jenkinsci/bom run passed without it. If it turns out this is still needed, proprietary consumers can pass it in with -D when invoking PCT. But please try not to.

basil · 2023-02-08T01:59:00Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

@@ -252,7 +249,7 @@ private void testPluginAgainst(

        File pluginCheckoutDir =
                new File(
-                        config.workDirectory.getAbsolutePath()
+                        config.getWorkingDir().getAbsolutePath()


For some reason this never had a getter before. When cleaning up this code I made the field private and added a getter for consistency.

basil · 2023-02-08T01:59:07Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

+                    File localCheckoutDir = config.getLocalCheckoutDir();
+                    if (localCheckoutDir == null) {
+                        throw new AssertionError(
+                                "Could never happen, but needed to silence SpotBugs");
+                    }
                    LOGGER.log(
                            Level.INFO,
                            "Copy plugin directory from {0}",
-                            config.getLocalCheckoutDir().getAbsolutePath());
+                            localCheckoutDir.getAbsolutePath());
                    try {
                        org.codehaus.plexus.util.FileUtils.copyDirectoryStructure(
-                                config.getLocalCheckoutDir(), pluginCheckoutDir);
+                                localCheckoutDir, pluginCheckoutDir);


Just making SpotBugs happy.

basil · 2023-02-08T02:07:32Z

src/main/java/org/jenkins/tools/test/maven/ExternalMavenRunner.java

        }
-        for (Map.Entry<String, String> entry : config.userProperties.entrySet()) {
-            cmd.add("--define=" + entry);
+        for (Map.Entry<String, String> entry : properties.entrySet()) {


Notice that this is now based on the parameter rather than a global set of properties for all Maven invocations. And the properties map that we pass in during compilation is almost always just skipping Javadoc generation, while the properties map that we pass in during test execution is what the user specified with -D when invoking PCT as well as the things like -overrideWar, jenkins.version, etc.

basil · 2023-02-08T02:08:01Z

src/main/java/org/jenkins/tools/test/model/PluginCompatTesterConfig.java

    // The megawar
-    private File war;
+    @NonNull private final File war;


What looks like needless churn in this file is just sorting these fields, getters, and setters in the same order as they appear in the main CLI Java source file.

basil · 2023-02-08T02:08:21Z

src/test/java/org/jenkins/tools/test/hook/JacocoHookTest.java

-                                "hpi:resolve-test-dependencies",
-                                "hpi:test-hpl",
-                                "surefire:test")));
+                        List.of("hpi:resolve-test-dependencies", "hpi:test-hpl", "surefire:test")));


Adapting to the fact that we're no longer in the business of managing forkCount.

basil · 2023-02-08T02:08:25Z

src/test/java/org/jenkins/tools/test/hook/WarningsNGExecutionHookTest.java

-                                "hpi:resolve-test-dependencies",
-                                "hpi:test-hpl",
-                                "surefire:test")));
+                        List.of("hpi:resolve-test-dependencies", "hpi:test-hpl", "surefire:test")));


Adapting to the fact that we're no longer in the business of managing forkCount.

basil · 2023-02-08T02:08:38Z

src/test/java/org/jenkins/tools/test/hook/WarningsNGExecutionHookTest.java

-
-    @Test
-    public void testType() {
-        final WarningsNGExecutionHook hook = new WarningsNGExecutionHook();
-
-        Map<String, Object> info = new HashMap<>();
-        info.put("types", new ArrayList<>(List.of("surefire")));
-        Map<String, Object> afterAction = hook.action(info);
-        List<String> types = (List<String>) afterAction.get("types");
-        assertThat(types.size(), is(2));
-        assertTrue(types.contains("failsafe"));
-    }


Removing a test for unused functionality that existed only for the XML report which is now ripped out.

jtnord

can no longer test all plugins in a war by ommiting --include-plugins

src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java

jtnord · 2023-02-08T10:55:05Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

+        File localCheckoutDir = config.getLocalCheckoutDir();
+        return localCheckoutDir != null && localCheckoutDir.exists();


:-o #449 filed - this is fine here.

src/main/java/org/jenkins/tools/test/model/PluginCompatTesterConfig.java

Co-authored-by: James Nord <jtnord@users.noreply.github.com>

basil added the work-in-progress label Feb 7, 2023

basil force-pushed the cli branch 2 times, most recently from 1cd4d1f to 6cc362a Compare February 7, 2023 23:05

basil added a commit to basil/bom that referenced this pull request Feb 7, 2023

Test jenkinsci/plugin-compat-tester#448

2cd5f75

basil force-pushed the cli branch from 6cc362a to a79fb8d Compare February 7, 2023 23:44

basil added breaking and removed work-in-progress labels Feb 8, 2023

basil commented Feb 8, 2023

View reviewed changes

basil requested a review from jtnord February 8, 2023 02:08

basil marked this pull request as ready for review February 8, 2023 02:08

basil requested a review from a team as a code owner February 8, 2023 02:08

imonteroperez approved these changes Feb 9, 2023

View reviewed changes

basil force-pushed the cli branch from 6e8154d to ec9a9a9 Compare February 9, 2023 23:58

Clean up CLI

7c9c23b

basil force-pushed the cli branch from ec9a9a9 to 7c9c23b Compare February 13, 2023 17:04

jtnord requested changes Feb 14, 2023

View reviewed changes

basil and others added 3 commits February 14, 2023 11:48

Update src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java

1280538

Co-authored-by: James Nord <jtnord@users.noreply.github.com>

fixups

4c5608c

spotless

fdbf41b

basil merged commit b0641fa into master Feb 14, 2023

basil deleted the cli branch February 14, 2023 19:58

This was referenced Feb 14, 2023

Remove failIfNoTests=false #430

Closed

Upgrade PCT to latest jenkinsci/bom#1735

Merged

basil added a commit that referenced this pull request Feb 15, 2023

Fixups to "Clean up CLI (#448)"

383f3ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up CLI #448

Clean up CLI #448

basil commented Feb 7, 2023 •

edited

Loading

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

basil Feb 8, 2023

jtnord left a comment

jtnord Feb 8, 2023

		File localCheckoutDir = config.getLocalCheckoutDir();
		return localCheckoutDir != null && localCheckoutDir.exists();

Clean up CLI #448

Clean up CLI #448

Conversation

basil commented Feb 7, 2023 • edited Loading

Testing done

How to adapt consumers

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

jtnord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil commented Feb 7, 2023 •

edited

Loading