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

Flatten Maven project #386

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Flatten Maven project #386

merged 1 commit into from
Jan 11, 2023

Conversation

basil
Copy link
Member

@basil basil commented Nov 22, 2022

Problem

This component originally had a number of Maven modules: a model which was then consumed by a CLI and a Google App Engine client. The Google App Engine client was ripped out at some point, so now the multi-module build remains, even though in practice there is really just one artifact that is produced by this repository, which is the PCT command-line interface (CLI). I could not find any consumers of any artifacts other than the CLI after searching usages in sources and binaries in both the jenkinsci and cloudbees GitHub organizations. As such, the split into multiple modules just makes it more difficult to edit the code and update dependencies.

Solution

Flatten the project into a single CLI module.

Implementation

I essentially just merged the contents of the POM files together. There is no new code here, just moved code.

Testing done

Testing in jenkinsci/bom complete at jenkinsci/bom#1678. I am hoping that James can run the same test for any proprietary consumers.

@basil basil force-pushed the flatten branch 2 times, most recently from c7641da to 23fcfba Compare November 22, 2022 03:38
basil added a commit to basil/bom that referenced this pull request Jan 10, 2023
@basil basil marked this pull request as ready for review January 10, 2023 01:58
@basil basil requested a review from a team as a code owner January 10, 2023 01:58
Comment on lines +31 to +32
<maven.scm.providers.version>1.5</maven.scm.providers.version>
<trimStackTrace>false</trimStackTrace> <!-- SUREFIRE-1798 -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from plugins-compat-tester/pom.xml.

Copy link
Member

Choose a reason for hiding this comment

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

BTW I think this is either inherited from the parent POM, or (if the Surefire version is inherited) obsolete.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW you never found the time to follow up on #348 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

😕 Is that somehow related to trimStackTrace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is trimStackTrace somehow related to flattening the Maven project? 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Just to the extent that it is POM text you are copying, and you might not have been aware that it is (IIRC) obsolete and could probably be omitted from the copy. If you did know and do not care, then you can simply ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

A workaround you introduced in jenkinsci/plugin-pom#33 and which propagated to other components, and which you never removed once it was no longer needed? Not of concern to me. Do not bother me with such things, especially if you are not willing to follow up on #348 (comment) and similar.

Comment on lines +57 to +61
<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>annotations</artifactId>
<version>3.0.1u2</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from plugins-compat-tester-model/pom.xml.

Comment on lines +87 to +91
<dependency>
<groupId>net.java.dev.jna</groupId>
<artifactId>jna</artifactId>
<version>4.1.0</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

Added to resolve an upper bounds conflict.

Comment on lines +315 to +369
<build>
<finalName>${project.artifactId}</finalName>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<version>3.2.2</version>
<configuration>
<archive>
<manifestEntries>
<Add-Opens>java.base/java.lang.reflect java.base/java.text java.base/java.util java.desktop/java.awt.font</Add-Opens>
</manifestEntries>
</archive>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-plugin</artifactId>
<version>3.2.2</version>
<executions>
<execution>
<goals>
<goal>shade</goal>
</goals>
<phase>package</phase>
<configuration>
<createDependencyReducedPom>false</createDependencyReducedPom>
<transformers>
<transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer">
<mainClass>org.jenkins.tools.test.PluginCompatTesterCli</mainClass>
</transformer>
<transformer implementation="org.apache.maven.plugins.shade.resource.ComponentsXmlResourceTransformer" />
<transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer" />
<transformer implementation="org.apache.maven.plugins.shade.resource.AppendingTransformer">
<resource>META-INF/spring.handlers</resource>
</transformer>
<transformer implementation="org.apache.maven.plugins.shade.resource.AppendingTransformer">
<resource>META-INF/spring.schemas</resource>
</transformer>
</transformers>
<filters>
<filter>
<artifact>*:*</artifact>
<excludes>
<exclude>META-INF/*.SF</exclude>
<exclude>META-INF/*.RSA</exclude>
</excludes>
</filter>
</filters>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from plugins-compat-tester-cli/pom.xml.

@@ -115,11 +123,179 @@
</dependencyManagement>

<dependencies>
<dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

Everything added here was moved from one of the other pom.xml files.

@basil basil requested a review from jtnord January 10, 2023 05:53
@jglick jglick added the chore label Jan 10, 2023
@jglick
Copy link
Member

jglick commented Jan 10, 2023

If merged, then after release RPU could be cleaned up (jenkins-infra/repository-permissions-updater#2827).

See #380 / #381: this will be incompatible for the CB usage since we depend on one of the modules in order to write custom hooks. I expect migration should be simple (just switching artifactId in the dep).

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Thanks! This does seem like a long-overdue simplification.

</dependency>


<!--TODO(oleg_nenashev): Remove all providers except Git? -->
Copy link
Member

Choose a reason for hiding this comment

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

Note: merge conflict with #382.

Comment on lines +31 to +32
<maven.scm.providers.version>1.5</maven.scm.providers.version>
<trimStackTrace>false</trimStackTrace> <!-- SUREFIRE-1798 -->
Copy link
Member

Choose a reason for hiding this comment

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

BTW I think this is either inherited from the parent POM, or (if the Surefire version is inherited) obsolete.

@jtnord
Copy link
Member

jtnord commented Jan 10, 2023

I could not find any consumers of any artifacts other than the CLI after searching usages in sources and binaries in both the jenkinsci and cloudbees GitHub organizations

we have some specific hooks that we then use when running the CLI.

Will file a PR and see what I can not break.

@jtnord
Copy link
Member

jtnord commented Jan 11, 2023

local code adapted and seems to be progressing well.

@jglick
Copy link
Member

jglick commented Jan 11, 2023

@basil I think you have permission to merge? If not, I will.

@basil
Copy link
Member Author

basil commented Jan 11, 2023

local code adapted and seems to be progressing well.

Great!

I think you have permission to merge? If not, I will.

I do not require your assistance.

@basil basil added internal and removed chore labels Jan 11, 2023
@basil basil merged commit 8bdba69 into jenkinsci:master Jan 11, 2023
@basil basil deleted the flatten branch January 11, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants