Prepare PCT for workflow-cps becoming a multi-module project #383

dwnusbaum · 2022-11-02T20:53:41Z

jenkinsci/workflow-cps-plugin#612 refactors workflow-cps into a multi-module project that contains groovy-cps and groovy-cps-dgm-builder. Without any other changes, this breaks the PCT, so this PR tries to keep things working.

Once jenkins-infra/repository-permissions-updater#2917 is merged we will be able to test this without -localCheckoutDir to make sure things still work correctly.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

…not compiled twice

dwnusbaum · 2022-11-02T20:55:33Z

...ns-compat-tester/src/main/java/org/jenkins/tools/test/hook/NodeCleanupBeforeCompileHook.java

-        System.out.println("Compile plugin log in " + path);
-        File compilePomLogfile = new File(path + "/compilePluginLog.log");
-        runner.run(mavenConfig, path, compilePomLogfile, "clean", "process-test-classes", "-Dmaven.javadoc.skip");


As far as I can tell, this was just a bug in #247 which caused sse-gateway and workflow-cps to be compiled twice. My understanding is that if you want to override the default compilation from a hook, you need to add the OVERRIDE_DEFAULT_COMPILE flag to the info map passed to action.

Did you check that sse-gateway is still OK?

Yeah, but that was before 6f446ae. I confirmed that previously it got compiled twice. I will check again though. I'm not really sure why this hook even exists, because I don't think these are the only two plugins that use frontend-maven-plugin, and a quick look at #247 did not clarify the underlying issue.

(I am retesting sse-gateway now, but NPM is extremely slow, maybe because sse-gateway is using very old versions of node and npm and so everything is running via Rosetta on my machine)

I confirmed that sse-gateway still works.

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHooks.java

dwnusbaum · 2022-11-02T20:57:13Z

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/MultiParentCompileHook.java

@@ -117,7 +117,11 @@ private MavenRunner.Config getMavenConfig(PluginCompatTesterConfig config) throw
    }

    private void compile(MavenRunner.Config mavenConfig, File path, File localCheckoutDir, String parentFolder, String pluginName) throws PomExecutionException, IOException {
-        if (isSnapshotMultiParentPlugin(parentFolder, path, localCheckoutDir)) {
+        if (pluginName.equals("workflow-cps")) {
+            // TODO: Why does the following condition not work for workflow-cps? Is it only broken for local checkouts?


I am not sure about this. Maybe it is just the case that -localCheckoutDir has never worked correctly with multi-module projects? I will look into this a bit more to understand if this is something that should be more general or if there is something that should be changed in the isSnapshotMultiParentPlugin and/or getMavenModule methods.

Maybe it is just the case that -localCheckoutDir has never worked correctly with multi-module projects?

I would not be surprised. Everything about MultiParentCompileHook is dumb. This part of PCT should be rewritten so that no hooks are needed—should collect repos corresponding to selected plugins, then run mvn commands from repo root.

Ok, yeah it looks like this is a bit of a mess. You need this and WorkflowCpsHooks$OnlyTestPluginHook when you are using -localCheckoutDir. However, when you do not use -localCheckoutDir, this new code and OnlyTestPluginHook make the PCT fail and must be skipped!

My takeaway is that -localCheckoutDir does not work for multi-module projects right now. Since that option is not used in typical workflows, I will revert the changes related to it and just file an issue about the behavior of -localCheckoutDir with multi-module projects.

6f446ae simplifies things to only work when not using -localCheckoutDir.

#384 tracks the -localCheckoutDir issue.

dwnusbaum · 2022-11-02T21:00:28Z

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHooks.java

+                    args.add("-pl");
+                    args.add("plugin");


Running groovy-cps's tests is quick and relatively harmless, but since it doesn't have plugin dependencies, testing it in the PCT is not very useful. The main reason for this is that without it, testing groovy-cps fails because groovy-cps-dgm-builder cannot be found in the local Maven repository due to jenkinsci/workflow-cps-plugin@1b01482. Another option would be be to modify the goal to install and recompile the plugin and the libraries a second time so that we can test everything.

modify the goal to install

I thought that was what you were already doing in this PR. 🤷 whatever works

The PCT compiles the plugin with one invocation of Maven and then runs tests separately with another. Compilation typically runs process-test-classes, but for multi-module projects (and workflow-cps as of the new logic in this PR) runs install. Tests are normally run using individual test goals like surefire:test (and this PR tweaks that invocation to add -pl plugin for workflow-cps). I am not sure why compilation and testing is split like this, but I do not work with the PCT very much.

I am not sure why compilation and testing is split like this

It is mostly a relic of the old mode of PCT predating jenkinsci/maven-hpi-plugin#336. The idea was to be able to mvn test or mvn install in one pass with suitable properties. Unfortunately for technical reasons you need to define jenkins.version during at least the test phase, meaning that the compilation would also be against the new version, which we do not want. So we would want a PCT run (incl. multimodule) to look like

mvn -Pquick-build install mvn -Djenkins.version=… -Dwhatever.war.arg=… hpi:resolve-test-dependencies surefire:test

(so that the second command does not recompile anything).

jglick · 2022-11-02T21:32:09Z

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/MultiParentCompileHook.java

@@ -117,7 +117,11 @@ private MavenRunner.Config getMavenConfig(PluginCompatTesterConfig config) throw
    }

    private void compile(MavenRunner.Config mavenConfig, File path, File localCheckoutDir, String parentFolder, String pluginName) throws PomExecutionException, IOException {
-        if (isSnapshotMultiParentPlugin(parentFolder, path, localCheckoutDir)) {
+        if (pluginName.equals("workflow-cps")) {
+            // TODO: Why does the following condition not work for workflow-cps? Is it only broken for local checkouts?


Maybe it is just the case that -localCheckoutDir has never worked correctly with multi-module projects?

I would not be surprised. Everything about MultiParentCompileHook is dumb. This part of PCT should be rewritten so that no hooks are needed—should collect repos corresponding to selected plugins, then run mvn commands from repo root.

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/MultiParentCompileHook.java

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHooks.java

jglick · 2022-11-02T21:36:04Z

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHooks.java

+                    args.add("-pl");
+                    args.add("plugin");


modify the goal to install

I thought that was what you were already doing in this PR. 🤷 whatever works

…ust use AbstractMultiParentHook

jglick

Looks fine. Do you need this released?

jglick · 2022-11-03T17:59:48Z

...ns-compat-tester/src/main/java/org/jenkins/tools/test/hook/NodeCleanupBeforeCompileHook.java

-        System.out.println("Compile plugin log in " + path);
-        File compilePomLogfile = new File(path + "/compilePluginLog.log");
-        runner.run(mavenConfig, path, compilePomLogfile, "clean", "process-test-classes", "-Dmaven.javadoc.skip");


Did you check that sse-gateway is still OK?

plugins-compat-tester/src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHook.java

Co-authored-by: Jesse Glick <[email protected]>

dwnusbaum · 2022-11-03T18:16:01Z

Do you need this released?

I think so? I know of one usage of this repository as a Git submodule, so IDK if releases matter in that context. Not sure about https://github.com/jenkinsci/bom.

jglick · 2022-11-03T18:30:34Z

Releases would not matter for submodule usage. bom can accept an incremental version for pretesting; it would be a good idea to open a bom draft PR with this and jenkinsci/workflow-cps-plugin#612.

dwnusbaum · 2022-11-03T19:04:07Z

See jenkinsci/bom#1553.

dwnusbaum · 2022-11-04T16:24:59Z

@jglick jenkinsci/bom#1553 is passing with the antisamy-markup-formatter update, so I think this can be merged now.

dwnusbaum added 2 commits November 2, 2022 16:37

Fix NodeCleanupBeforeCompileHook so sse-gateway and workflow-cps are …

24bad3d

…not compiled twice

Add support for workflow-cps as a multi-module project

eb3c171

dwnusbaum commented Nov 2, 2022

View reviewed changes

Add newline to end of file

4225470

This was referenced Nov 2, 2022

Merge groovy-cps into workflow-cps jenkinsci/workflow-cps-plugin#612

Merged

Add permissions files for groovy-cps and workflow-cps-parent jenkins-infra/repository-permissions-updater#2917

Merged

jglick approved these changes Nov 2, 2022

View reviewed changes

dwnusbaum added 2 commits November 2, 2022 17:59

Remove unused code to fix SpotBugs warnings

c189251

Ignore problems with localCheckoutDir and multi-module projects and j…

6f446ae

…ust use AbstractMultiParentHook

dwnusbaum marked this pull request as ready for review November 3, 2022 17:41

dwnusbaum requested a review from a team as a code owner November 3, 2022 17:41

jglick approved these changes Nov 3, 2022

View reviewed changes

jglick added the enhancement label Nov 3, 2022

Use equals instead of equalsIgnoreCase

72d48d4

Co-authored-by: Jesse Glick <[email protected]>

dwnusbaum mentioned this pull request Nov 3, 2022

-localCheckoutDir does not work with multi-module projects #384

Closed

dwnusbaum mentioned this pull request Nov 3, 2022

Update workflow-cps, antisamy-markup-formatter, and the PCT jenkinsci/bom#1553

Merged

jglick merged commit b2c7632 into jenkinsci:master Nov 4, 2022

dwnusbaum deleted the workflow-cps branch November 4, 2022 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare PCT for workflow-cps becoming a multi-module project #383

Prepare PCT for workflow-cps becoming a multi-module project #383

dwnusbaum commented Nov 2, 2022 •

edited

Loading

dwnusbaum Nov 2, 2022 •

edited

Loading

jglick Nov 3, 2022

dwnusbaum Nov 3, 2022

dwnusbaum Nov 3, 2022

dwnusbaum Nov 3, 2022

dwnusbaum Nov 2, 2022

jglick Nov 2, 2022

dwnusbaum Nov 3, 2022

dwnusbaum Nov 3, 2022

dwnusbaum Nov 3, 2022

dwnusbaum Nov 2, 2022

jglick Nov 2, 2022

dwnusbaum Nov 2, 2022

jglick Nov 2, 2022

jglick Nov 2, 2022

jglick Nov 2, 2022

jglick left a comment

jglick Nov 3, 2022

dwnusbaum commented Nov 3, 2022

jglick commented Nov 3, 2022

dwnusbaum commented Nov 3, 2022

dwnusbaum commented Nov 4, 2022

Prepare PCT for workflow-cps becoming a multi-module project #383

Prepare PCT for workflow-cps becoming a multi-module project #383

Conversation

dwnusbaum commented Nov 2, 2022 • edited Loading

dwnusbaum Nov 2, 2022 • 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

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

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum commented Nov 3, 2022

jglick commented Nov 3, 2022

dwnusbaum commented Nov 3, 2022

dwnusbaum commented Nov 4, 2022

dwnusbaum commented Nov 2, 2022 •

edited

Loading

dwnusbaum Nov 2, 2022 •

edited

Loading