jetty.project icon indicating copy to clipboard operation
jetty.project copied to clipboard

Revert change in 12.1.x that puts ee# version properties in top level pom

Open joakime opened this issue 1 year ago • 1 comments

Jetty version(s) 12.1.x

Jetty Environment All

Java version/vendor (use: java -version) Any

OS type/version Any

Description There is a change in the top level pom in branch jetty-12.1.x that puts all of the EE# property versions in the top level pom.

  • Commit https://github.com/jetty/jetty.project/commit/5dc9a73970de125b34ed3d757457ee76b80e2d3c

https://github.com/jetty/jetty.project/blob/efc20894adc431493b92c84aad0b3115f0fd3447/pom.xml#L183-L272

This has to be undone as it doesn't play nicely with any of the versioning tools (command line, maven, IDE, dependabot, rennovate, etc) or CVE tooling.

Each of those tools works with the /jetty-ee#/ tree in isolation (as we don't want changes in /jetty-ee9/ to impact /jetty-ee10/ for example). Putting these properties in the top level pom hides them from all of those tools, and those versions cannot be updated in isolation from each other.

Also, In the future, if we go with git submodules, this change cannot exist.

This also prevents merging of jetty-12.0.x versions in a reliable way.

joakime avatar Jun 27 '24 11:06 joakime

This has to be undone as it doesn't play nicely with any of the versioning tools (command line, maven, IDE, dependabot, rennovate, etc) or CVE tooling.

Please provide a concrete example of a problem.

Each of those tools works with the /jetty-ee#/ tree in isolation (as we don't want changes in /jetty-ee9/ to impact /jetty-ee10/ for example).

Changes in jetty-ee9 are still absolutely isolated and have no impact on any other module. Nothing about this change impacts in any way the build relationship between any existing modules.

Putting these properties in the top level pom hides them from all of those tools, and those versions cannot be updated in isolation from each other.

Please provide a concrete example, and an explanation of why these tools cannot be reconfigured to refer to the top level pom properties instead.

Also, In the future, if we go with git submodules, this change cannot exist.

Not an argument for the present. Moreover, all the jetty-eeX modules already depend on properties that are declared in the top level pom, so if we do go to git submodules in the future we would have to face this problem anyway.

This also prevents merging of jetty-12.0.x versions in a reliable way.

Merging from jetty-12.0.x is already problematic, so I don't think this is a showstopper.

I'm willing to look at other solutions, but the solution is not to duplicate dependency version information.

janbartel avatar Jun 28 '24 03:06 janbartel

@joakime can you take a stab at writing a policy of where properties should be defined as currently we have a mix and it is not as simple as reverting the current change.

This is a high priority for 12.1.0 as it will be hard to change after that release.

gregw avatar Apr 09 '25 21:04 gregw

I find it a bit confusing to have in jetty-eexx/pom.xml something such.

<jakarta.activation.api.version>${ee11.jakarta.activation.api.version}</jakarta.activation.api.version>

then later in the pom

<dependency>
  <groupId>jakarta.activation</groupId>
  <artifactId>jakarta.activation-api</artifactId>
  <version>${jakarta.activation.api.version}</version>
</dependency>

why not simply

<dependency>
  <groupId>jakarta.activation</groupId>
  <artifactId>jakarta.activation-api</artifactId>
  <version>${ee11.jakarta.activation.api.version}</version>
</dependency>

The first (and double interpolation) looks overcomplicated for no real gain (or did I miss something?) Furthermore, some of those poms can be very simplified to avoid duplication of core dependencies, which can be simply declared in the top pom dependencyManagement and not duplicated in child poms. I'd like to try using the Jakarta Platform BOM for EE11 to avoid some of the entry declarations.

And it's even a bit more complicated with mod file

[ini]
ee11.jakarta.authentication.api.version?=@jakarta.authentication.api.version@

[lib]
lib/jetty-ee11-jaspi-${jetty.version}.jar
lib/ee11-jaspi/jakarta.authentication-api-${ee11.jakarta.authentication.api.version}.jar

can be as simple as (not really sure someone with start jetty with overriding the value via command line)

[lib]
lib/jetty-ee11-jaspi-${jetty.version}.jar
lib/ee11-jaspi/jakarta.authentication-api-@[email protected]

olamy avatar Apr 25 '25 23:04 olamy

draft PR https://github.com/jetty/jetty.project/pull/13052 (not complete as changed made only for ee11) but this looks to remove some XML content :)

Image

olamy avatar Apr 27 '25 02:04 olamy

I find it a bit confusing to have in jetty-eexx/pom.xml something such.

<jakarta.activation.api.version>${ee11.jakarta.activation.api.version}</jakarta.activation.api.version>

then later in the pom

<dependency>
  <groupId>jakarta.activation</groupId>
  <artifactId>jakarta.activation-api</artifactId>
  <version>${jakarta.activation.api.version}</version>
</dependency>

why not simply

<dependency>
  <groupId>jakarta.activation</groupId>
  <artifactId>jakarta.activation-api</artifactId>
  <version>${ee11.jakarta.activation.api.version}</version>
</dependency>

Because that was the least disruptive change - otherwise I would have had to edit everywhere that jakarta.activation.api.version was referenced and replace it with ee11.jakarta.activation.api.version. But if you want to fully work through that change, fine.

janbartel avatar Apr 27 '25 02:04 janbartel

In an ideal world, we wouldn't need all those properties but just use a bom containing all those APIS declared at the top pom of each eexx. (and each demos) (if we need a minor upgrade of any API it's possible to override it by adding the dependency in dependencyManagement same section as the bom) The PR have started to do those changes. Now is the problem of mod files. Instead of having something such:

[lib]
lib/[email protected]@.jar

We should be able to declare something such

lib/@file:jakarta.servlet:jakarta.servlet-api@

format such

@file:groupId:artifcatId[:jar]@

The home-made interpolator (similar to the one we already have for ee9->ee8) will understand this format and transform to the correct format artifactId-version.jar (version coming from the project dependencies)

This will seriously reduce size of the pom and so the maintenance.

olamy avatar Apr 30 '25 05:04 olamy

ok, changes have been implemented (reducing a large number of properties) Now properties can even be moved to each subdirectory jetty-eexx The only needed at the top are the bom version (such as <ee11.jakarta.bom.version>11.0.0</ee11.jakarta.bom.version>) because jetty-demos use it mod files are now accepting a the following format

lib/ee10-annotations/@jakarta.annotation:jakarta.annotation-api@
$path@groupId:artifactId@

I implemented changes for this only for jakarta/javax jars but can be done for all.

olamy avatar May 06 '25 10:05 olamy

We have traditionally used the org.codehaus.mojo:versions-maven-plugin with the update-properties goal. Do they still work?

See: the various profiles with update-dependencies and update-dependencies-<env>, these are typically kicked off with a script.

See the details of what profile names are used within the script files ...

If they don't work, why? Is there a better solution? (a different plugin? a different properties layout in our project? etc)

joakime avatar May 06 '25 22:05 joakime

There is a test build with current changes (not moving ee properties) here https://jenkins.webtide.net/job/nightlies/job/reports/job/jetty-12.1-dependency-report/34/ But sure some needs to be adjusted/removed when/if moving properties to sub project.

olamy avatar May 06 '25 22:05 olamy

Both #13052 and #13204 is merged. Closing.

joakime avatar Jun 04 '25 21:06 joakime