use new toolchain parent #6
Conversation
Signed-off-by: Olivier Lamy <[email protected]>
| <parent> | ||
| <groupId>org.eclipse.jetty.toolchain</groupId> | ||
| <artifactId>jetty-toolchain</artifactId> | ||
| <version>1.7</version> | ||
| <version>1.8-parent-SNAPSHOT</version> |
There was a problem hiding this comment.
The child repos should never depend on a SNAPSHOT parent.
Depending on a SNAPSHOT complicates the release process.
This will also allow dependabot to update the parent versions.
AND we'll see when a plugin version update breaks the build of the child repo (which has happened countless times before).
But now we will have to manually fix the incompatible plugin version.
There was a problem hiding this comment.
@joakime It's a draft/working in progress test ONLY branch.
c'mon I will not release the parent immediately...
| <compiler.source>17</compiler.source> | ||
| <compiler.target>17</compiler.target> | ||
| <compiler.release>17</compiler.release> | ||
| <jdk.version.minimum>17</jdk.version.minimum> |
There was a problem hiding this comment.
Where did this property name come from?
Is it one of the bog standard Maven properties?
If not, then this will break some IDE users. (the IDE will not know what JDK version to use)
There was a problem hiding this comment.
please look at the new parent pom: https://github.com/jetty/jetty.toolchain/blob/7ca85e650cb2b83c80530639685cd5e1255d78b8/pom.xml#L199
pom.xml
Outdated
| <packaging>jar</packaging> | ||
| <name>Jetty :: SetUID JNA</name> | ||
| <scm child.scm.connection.inherit.append.path="false" child.scm.developerConnection.inherit.append.path="false" child.scm.url.inherit.append.path="false"> |
There was a problem hiding this comment.
This syntax (introduced in Maven 3.6.1) has broken several IDE integrations.
(mainly because the official maven xsd wasn't versioned and the IDEs were surprised by the updated xsd to support this)
IIRC, users on older maven versions will see odd failures if this is in use.
Why can't we just use the more normal ...
<properties>
<project.directory>${project.artifactId}</project.directory>
</properties>
<scm>
<developerConnection>scm:git:[email protected]:jetty/</developerConnection>
</scm>oh, as I write this I realize the artifactId and the repo name are not in sync. grumble
There was a problem hiding this comment.
I will remove those attributes as my first idea was simply to not have any <scm> and just setup this <jetty.git.repo>jetty.setuid</jetty.git.repo>
But I figure this will probably not work with m-release-p
Yes I would definitely be happy to use ${project.artifactId} but the git repos have not been created with such naming conventions...
| <enabled>true</enabled> | ||
| </snapshots> | ||
| <id>jetty.snapshots</id> | ||
| <url>https://oss.sonatype.org/content/repositories/jetty-snapshots</url> |
There was a problem hiding this comment.
No, we should not do this.
The maven local repo caching between builds doesn't handle this very well.
We used to do this in jetty-project/ repos, and would get nice updates for our staged jetty releases, and also updates for snapshots on jetty-test-helper.
But rerolls or staged releases and updated SNAPSHOTS both used the old maven versions (even if they were updated days / weeks later)
We've ripped out this section from our jetty-project/ repos because of the weirdness here.
We shouldn't be reintroducing this.
There was a problem hiding this comment.
AGAIN it's a temporary working branch as the parent is not released yet and so GHA build would have fail.
| <enabled>true</enabled> | ||
| </snapshots> | ||
| <id>jetty.snapshots</id> | ||
| <url>https://oss.sonatype.org/content/repositories/jetty-snapshots</url> |
There was a problem hiding this comment.
If you want these in the pom, then they should be behind a profile, not default.
This will, at least, keep dependabot from using them.
| <rules> | ||
| <requireMavenVersion> | ||
| <version>[3.8.4,)</version> | ||
| <message>[ERROR] OLD MAVEN [${maven.version}] in use, Jetty ${project.version} requires Maven 3.8.4 or newer</message> |
There was a problem hiding this comment.
This enforcer configuration NEEDS to be at the child repo level.
Not in a parent pom.
There was a problem hiding this comment.
it's using a property so you can override it
| <configuration> | ||
| <useReleaseProfile>true</useReleaseProfile> | ||
| <releaseProfiles>eclipse-release</releaseProfiles> | ||
| <autoVersionSubmodules>true</autoVersionSubmodules> |
There was a problem hiding this comment.
Last time we tried this, the parent <configuration> didn't merge with the child repo, if we wanted something specific in the child repo, we couldn't just add a single configuration difference, we had to copy/paste the whole <configuration> to the child repo and then make edits.
There was a problem hiding this comment.
maybe it will not work for 1 or 2 of 20 or more repos but it will be still a gain
do you have an example I'm curious to see what may not work to override something in child poms
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
|
let's forget that! copy/paste is the modern tooling for 2024 |
Signed-off-by: Olivier Lamy [email protected]