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

Native support for multi-module Maven projects #488

Closed
basil opened this issue Mar 13, 2023 · 11 comments
Closed

Native support for multi-module Maven projects #488

basil opened this issue Mar 13, 2023 · 11 comments
Assignees

Comments

@basil
Copy link
Member

basil commented Mar 13, 2023

The current code has extremely special and convoluted handling for multi-module Maven projects. Every multi-module Maven project needs a custom hook in PCT in order to work at all, and even then there are various problems. For example, consider the case of a multi-module Maven project that contains a plugin with a dependency on another plugin in the same multi-module Maven project. An example of this is Pipeline: Declarative Extension Points API, which depends on Pipeline: Model API, both of which are in the same multi-module Maven project. In this case, our current workflow requires two different invocations of PCT, one for each plugin. However, this results in PCT unnecessarily compiling both the dependent and its dependency but only executing tests for the dependent.

At a high level, this should be rewritten so that given a set of plugins, PCT collects a set of repositories containing those plugins and then invokes Maven from the root of each repository. Then we would need no special handling for multi-module Maven projects.

Implementing this should be relatively straightforward: the list of plugins provided can be transformed into a list of repositories, with the main for loop in org.jenkins.tools.test.PluginCompatTester#testPlugins converted to loop over repositories rather than plugins, with the semantics of org.jenkins.tools.test.PluginCompatTester#testPluginAgainst changed to be "test repository against" rather than "test plugin against."

However, one complication is CI callers such as jenkinsci/bom. These callers take a megawar and list its plugins, invoking parallel runs of PCT, one for each plugin. With the change above to iterate over repositories over plugins, such consumers will want to pass in a list of all plugins that are contained in a given repository rather than passing in a single plugin. For example, to test the pipeline-model-definition-plugin repository, consumers would want to invoke PCT with --plugins pipeline-model-api,pipeline-model-definition,pipeline-model-extensions,pipeline-stage-tags-metadata; PCT should then coalesce these into the single repository pipeline-model-definition-plugin, do one clone, one build, and one test cycle. But how would consumers know to pass in that particular list of plugins?

One solution would be a new PCT flag that, for a given megawar, prints the mapping of repositories to plugins. For example, it could have output like this:

REPOSITORY                        PLUGINS
pipeline-model-definition-plugin  pipeline-model-api,pipeline-model-definition,pipeline-model-extensions,pipeline-stage-tags-metadata
text-finder-plugin                text-finder

Consumers would run PCT with this flag to determine the division of parallel branches, each line of the output being a parallel branch (one for each repository). The consumers would pass in the output of the PLUGINS column as the --plugins parameter to PCT in each branch.

@basil basil changed the title Support for multi-module Maven projects Native support for multi-module Maven projects Mar 14, 2023
@basil
Copy link
Member Author

basil commented Mar 14, 2023

https://github.com/jenkinsci/plugin-compat-tester/tree/multimodule (which I accidentally pushed to master, then reverted) implements the first half of this proposal (i.e., the first three paragraphs) and has been tested with PLUGINS=pipeline-model-api,pipeline-model-definition,pipeline-model-extensions,pipeline-stage-tags-metadata bash local-test.sh.

@jtnord Can you pick this up and take it across the finish line?

@jtnord
Copy link
Member

jtnord commented Mar 14, 2023

do one clone, one build

👍

and one test cycle

one test cycle per plugin?

Whilst we may be able to do something like mvn hpi:... surefire:test -pl pipeline-model-api,pipeline-model-definition my spider senses are tingling here as the project could be a dependency (that would get munged by the hpi-plugin and in the reactor.

changed to be "test repository against" rather than "test plugin against."

Would we ever be in a situation where the plugins from a single repository in the war would not be aligned?

Should not be the case for CloudBees' product - and I believe the tools bom is ok here to, in which case this should be ok.

@basil
Copy link
Member Author

basil commented Mar 14, 2023

one test cycle per plugin?

Yes.

Whilst we may be able to do something like mvn hpi:... surefire:test -pl pipeline-model-api,pipeline-model-definition

The code I have been hacking on so far in the multimodule branch currently does mvn -Djavadock.skip=true clean install -Pquick-build to do the build followed by mvn -DoverrideWar=[…] -Djenkins.version=[…] -DuseUpperBounds=true hpi:resolve-test-dependencies hpi:test-hpl surefire:test (without -pl) to do the test, and this seems to be working pretty well overall with the exception of (as you note) …

my spider senses are tingling here as the project could be a dependency (that would get munged by the hpi-plugin and in the reactor.

One step ahead of you, mate: yes indeed, as evidenced by these errors I got in some deeper testing today:

Mar 14, 2023 1:02:23 PM hudson.PluginWrapper versionDependencyError
WARNING: Suppressing dependency error in pipeline-model-extensions v999999-SNAPSHOT (private-31fd5b99-basil): Update required: Pipeline: Model API (pipeline-model-api 2.2118.v31fd5b_9944b_5) to be updated to 999999-SNAPSHOT or higher
--
Mar 14, 2023 1:02:34 PM hudson.PluginWrapper versionDependencyError
WARNING: Suppressing dependency error in pipeline-model-definition v999999-SNAPSHOT (private-31fd5b99-basil): Update required: Pipeline: Model API (pipeline-model-api 2.2118.v31fd5b_9944b_5) to be updated to 999999-SNAPSHOT or higher
Mar 14, 2023 1:02:34 PM hudson.PluginWrapper versionDependencyError
WARNING: Suppressing dependency error in pipeline-model-definition v999999-SNAPSHOT (private-31fd5b99-basil): Update required: Pipeline: Declarative Extension Points API (pipeline-model-extensions 2.2118.v31fd5b_9944b_5) to be updated to 999999-SNAPSHOT or higher
Mar 14, 2023 1:02:34 PM hudson.PluginWrapper versionDependencyError
WARNING: Suppressing dependency error in pipeline-model-definition v999999-SNAPSHOT (private-31fd5b99-basil): Update required: Pipeline: Stage Tags Metadata (pipeline-stage-tags-metadata 2.2118.v31fd5b_9944b_5) to be updated to 999999-SNAPSHOT or higher

But I was able to get those errors to go away with this code in maven-hpi-plugin: basil/maven-hpi-plugin@b718894

With the current prototype in the multimodule branch in this repository and in maven-hpi-plugin I have been able to successfully run PLUGINS=aws-java-sdk-cloudformation,aws-java-sdk-codebuild,aws-java-sdk-ec2,aws-java-sdk-ecr,aws-java-sdk-ecs,aws-java-sdk-efs,aws-java-sdk-elasticbeanstalk,aws-java-sdk-iam,aws-java-sdk-logs,aws-java-sdk-minimal,aws-java-sdk-sns,aws-java-sdk-sqs,aws-java-sdk-ssm,configuration-as-code,declarative-pipeline-migration-assistant,declarative-pipeline-migration-assistant-api,mina-sshd-api-common,mina-sshd-api-core,mina-sshd-api-scp,mina-sshd-api-sftp,pipeline-model-api,pipeline-model-definition,pipeline-model-extensions,pipeline-rest-api,pipeline-stage-tags-metadata,pipeline-stage-view,workflow-cps TEST=InjectedTest bash local-test.sh without any of the versionDependencyError messages shown above.

Here are the repositories that were cloned in the above run:

2023-03-14 20:12:33.716+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/aws-java-sdk-plugin.git at 8f993c987059dd7beb0b39429d6fc12275ada72d
2023-03-14 20:15:11.371+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/configuration-as-code-plugin.git at b72405b80249386de12951fb3df178657a132610
2023-03-14 20:15:52.582+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/declarative-pipeline-migration-assistant-plugin.git at declarative-pipeline-migration-assistant-1.5.5
2023-03-14 20:16:27.240+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/mina-sshd-api-plugin.git at a0e1f42659aa387162e3ec56558fd36c06fc516e
2023-03-14 20:17:19.008+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/pipeline-model-definition-plugin.git at 31fd5b9944b562e11892d8de7ea15d0cbb2cd564
2023-03-14 20:18:22.953+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/pipeline-stage-view-plugin.git at pipeline-stage-view-2.31
2023-03-14 20:19:09.794+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository https://github.com/jenkinsci/workflow-cps-plugin.git at 8177e69e359acf6718a289798d4ba08788bdf30c

In other words, a loop of 7 repositories rather than 27 plugins. This is probably going to cut down on compute cost.

I am feeling pretty good about this design right now. 😄

@jtnord jtnord self-assigned this Mar 22, 2023
basil added a commit to basil/bom that referenced this issue Mar 22, 2023
@basil
Copy link
Member Author

basil commented Mar 22, 2023

To facilitate collaboration, there is now a multimodule branch in both maven-hpi-plugin and plugin-compat-tester, with the latter consuming the latest incremental build from the former. I have retested the latest incrementals by running PLUGINS=aws-java-sdk-cloudformation,aws-java-sdk-codebuild,aws-java-sdk-ec2,aws-java-sdk-ecr,aws-java-sdk-ecs,aws-java-sdk-efs,aws-java-sdk-elasticbeanstalk,aws-java-sdk-iam,aws-java-sdk-logs,aws-java-sdk-minimal,aws-java-sdk-sns,aws-java-sdk-sqs,aws-java-sdk-ssm,configuration-as-code,declarative-pipeline-migration-assistant,declarative-pipeline-migration-assistant-api,mina-sshd-api-common,mina-sshd-api-core,mina-sshd-api-scp,mina-sshd-api-sftp,pipeline-model-api,pipeline-model-definition,pipeline-model-extensions,pipeline-rest-api,pipeline-stage-tags-metadata,pipeline-stage-view,workflow-cps TEST=InjectedTest bash local-test.sh successfully. I will now proceed to running a full automated test in jenkinsci/bom#1879.

@basil
Copy link
Member Author

basil commented Mar 23, 2023

Results doing automated testing in jenkinsci/bom are pretty good, leading the present author to conclude that an internal accusation that this work "is currently quite broken" was more than a bit overstated. The only failures observed in jenkinsci/bom testing were in tests in configuration-as-code's integrations/ directory, which we weren't even running before. The fact that we're running them now and getting useful feedback shows how much goodness is oozing out of this work.

@basil
Copy link
Member Author

basil commented Mar 23, 2023

And with jenkinsci/configuration-as-code-plugin#2234 all the tests in jenkinsci/bom are passing. From the jenkinsci/bom side the first three paragraphs of this issue are complete, leaving only the second three paragraphs.

@jtnord
Copy link
Member

jtnord commented Mar 24, 2023

mvn -Djavadock.skip=true clean install -Pquick-build to do the build followed by mvn -Dove

we must never install into the local repo - that will make a mockery of the PCT as we are no longer testing against the released binaries but what we just built and installed for other plugins outside of the reactor.
plugins built with java 8 produce different bytecode to ones built with java 11 (even with the -release flag).
Whilst for some things using CD this may work as those versions will never be used as dependencies for other repositories this is not the case for anything produced by maven-release-plugin .

This is especially true where this can be run on developer machines where we may not be running a single test for a single reactor in a throwaway environment.

@basil
Copy link
Member Author

basil commented Mar 24, 2023

This view could only be held by someone who has likely not tried to implement an alternative. This is not a code review, so if you feel that you could do a better job, then please push an improved version to the project branch (that also passes full jenkinsci/bom testing, as the current prototype does). In doing so, I think it is likely you will discover the reason this is not feasible.

@basil
Copy link
Member Author

basil commented Mar 25, 2023

we must never install into the local repo

I took a crack at this today and with some difficulty implemented most of it in jenkinsci/maven-hpi-plugin#453 and 3a5dee3f. More work needs to be done to adapt workflow-cps to support this use case. I am noting that I have not seen any contributions from you for multi-module support in maven-hpi-plugin or the multimodule branch of plugin-compat-tester. Given that this ticket is assigned to you, I look forward to your future contributions to this project.

basil added a commit to basil/bom that referenced this issue Mar 25, 2023
basil added a commit to basil/bom that referenced this issue Mar 26, 2023
basil added a commit to basil/bom that referenced this issue Apr 1, 2023
@basil
Copy link
Member Author

basil commented Apr 1, 2023

Weekly update: This project branch has passed its most rigorous automated test run yet in jenkinsci/bom: jenkinsci/bom#1893 with 552 passing checks on 4 BOM lines. And this is with released versions of all plugins (no incrementals, other than PCT itself).


I am not aware of contributions during the past week from any other member(s) of the project. If this situation persists, the project will stagnate.

@jtnord
Copy link
Member

jtnord commented Apr 19, 2023

included in 1313.v036de64e1863

@jtnord jtnord closed this as completed Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants