Revert top level ee# version properties change

[joakime]

Reverting top level ee# version properties, as it prevents the tooling built around versions to update the /jetty-ee#/ trees in isolation of each other.
Also, changed the /jetty-demos/ tree to depend on Jakarta EE, not Jetty EE.

[janbartel]

Please describe precisely how the definition of properties at the top level pom causes problems.

[janbartel]

This is precisely what I was trying to avoid - the duplication of dependency versions between the jetty-eeX poms and the jetty-demos.

[joakime]

Read the issue text

[joakime]

The jetty-demos don't depend on Jetty EE, they depend on Jakarta EE.
It was wrong to try to shoehorn them into Jetty EE.

[janbartel]

Read the issue text

I have.

The jetty-demos don't depend on Jetty EE, they depend on Jakarta EE. It was wrong to try to shoehorn them into Jetty EE.

The jetty-demos don't depend on any jetty-eeX module, they just depend on some of the same dependencies of those jetty-eeX modules.

The jetty-demos don't depend on Jetty EE, they depend on Jakarta EE. It was wrong to try to shoehorn them into Jetty EE.

I don't understand your argument here. There is no relationship between any jetty-eeX module and the jetty-demos modules. The jetty-eeX modules depend on Jakarta EE and so do the jetty-demos:

Servlet 4 javax.servlet api is depended on by: jetty-ee8 and jetty-servlet4-demos

Servlet 5 jakarta.servlet api is depended on by: jetty-ee9 and jetty-servlet5-demos

Servlet 6 jakarta.servlet api is depended on by: jetty-ee10

Servlet 6.1 jakarta.servlet.api is depended on by: jetty-ee11

If you want to declare the various servlet spec dependencies outside of the top level pom, say in servlet4/pom.xml, servlet5/pom.xml, servlet6/pom.xml, servlet6.1/pom.xml and then have jetty-ee8 and jetty-servlet4-demos depend on servlet4/pom.xml, jetty-ee9 and jetty-servlet5-demos depend on servlet5/pom.xml , etc etc then fine. That keeps the version information in a single place. My objection is to duplicating the version information across the project.

[joakime]

Please describe precisely how the definition of properties at the top level pom causes problems.

The environment trees /jetty-ee8/, /jetty-ee9/, /jetty-ee10/, and /jetty-ee11/ are isolated.
They have to be.

You should think of those trees as belonging to a different git repository for this to make sense.

I say those are isolated, as they do not participate in the global versioning of the entire repository.
They cannot, as they have different versions, often of the same maven coordinate!

If you yank out the actual version value (the 1.2.3 string, not the property ${foo.version}) then those are never seen in the isolation of those trees.

The structure of branch jetty-12.1.x HEAD right now is would make any tooling attempting to upgrade say /jetty-ee9/ not be able to see the versions coming from the root level pom.
They are not declared in the /jetty-ee#/ tree.

This is no different than if you had a git repo for project-foo that declared a parent against bar-parent (in a different repo) and the versions were declared in bar-parent, not in project-foo. The various version tooling against the dependencies (of project-foo) cannot see the version strings, nor recommend the upgrade, as the actual version strings are outside of the project.

The /jetty-demos/ tree should have used the Jakarta EE boms, not the Jetty EE maven project version properties. (and no, you don't want the /jetty-demos/ to use the Jetty EE boms).

[joakime]

Also, the failed build of this PR is due to a previous merge of the MultiPart stuff.

[ERROR]   MultiPartServletTest.testMultiPartGzip:475 » Completion java.lang.IllegalArgumentException: No files directory configured
[ERROR]   MultiPartServletTest.testMultiPartGzip:475 » Completion java.lang.IllegalArgumentException: No files directory configured

[gregw]

@joakime Jan asked me if I can explain what your concern is... I've had a look and a read and I have to say I'm not understanding what the problem is either, or at least not your explanation of it?

We already have dependencies with their version specified in the top level as a property, but used within a jetty-ee module. For example jboss.logging.version. Have the tools failed to work with that? Has that broken anything?
So if there is a problem, you are not making it clear what it is. What is it that actually breaks, and why has it not broken already on the versions that are in the top level? I also don't see the issue if we ultimately break up into separate repositories, as maven is perfectly capable of fetching a parent pom from a different repository and we have done so in the past.

I understand that we don't want global properties that affect all the different eeX's in some circumstances. But that's not what is happening in 12.1. Each jetty-eeX module still has its top level jakarta.servlet.api.version property that is set in its sub-module pom. But rather than being set by an explicit version, it is set from a global eeX.jakarta.servlet.api.version property. This is so that our separate modules that test/demo eeX can use the same version of the APIs that we actually implement. Are you saying that the issue is that the tools can handle a version that is set as a property, but not a version that is set as a property that is set as a property? If that is the issue, then it is a bug on the tooling.

I think it is a good feature that we can look in our top level pom and see:

     <ee8.jetty.servlet.api.version>4.0.6</ee8.jetty.servlet.api.version>
    <ee9.jakarta.servlet.api.version>5.0.0</ee9.jakarta.servlet.api.version>
    <ee10.jakarta.servlet.api.version>6.0.0</ee10.jakarta.servlet.api.version>
    <ee11.jakarta.servlet.api.version>6.1.0</ee11.jakarta.servlet.api.version>

Having these in the top level makes sure we implement and then test/demo the same things when we say eeX.

HOWEVER.... I do question the value of having all the non-API dependencies in the top level. These are things that are private to the implementation and have no need of coordination over multiple sub-modules. So perhaps we should move the non-api dependencies back to the sub-modules..... however, I do see that a lot of other such dependencies are already defined in the top level. So a clear policy for such dependencies should be defined. But I still don't see the actual problem is we decided to declare all our dependencies in the top level, so long as we avoid name clashes.

Note that, API version is something that a human needs to explicitly manage. EVEN IF the dependabot tools are broken by having API versions in the top level, I don't see that as a problem as Dependabot should never be doing an upgrade from servlet 6.1 to servlet 6.2.

Finally, in this PR, I see you are declaring some dependency version in the DependencyManagement clauses themselves. I really don't like mixing styles like that. I think all our dependency versions should be properties, even if only local or sub module ones. We should never need to go searching for a dependency version.... currently I look for a version in the properties of a pom.xml file, if I don't find it there, then I'm looking at the parent pom, not down the maybe long file at the dependency clauses themselves.

[joakime]

@joakime Jan asked me if I can explain what your concern is... I've had a look and a read and I have to say I'm not understanding what the problem is either, or at least not your explanation of it?

Unlike prior versions of Jetty (eg: Jetty 9 thru Jetty 11), our code tree is no longer viewed as a single reactor with simple dependency convergence.

Now we have separate scopes of dependencies.

• Core - which is:
  • Root Dir: /
  • Include: /pom.xml
  • Include: /jetty-core/
  • Include: /jetty-integrations/
  • Include: /tests/
  • Excluding /jetty-ee8/
  • Excluding /jetty-ee9/
  • Excluding /jetty-ee10/
  • Excluding /jetty-ee11/
• Jetty ee8: Root Dir: /jetty-ee8/
• Jetty ee9: Root Dir: /jetty-ee9/
• Jetty ee10: Root Dir: /jetty-ee10/
• Jetty ee11: Root Dir: /jetty-ee11/

For any tooling that relies on knowing or updating dependencies, each of those 5 bullet points represents a single configuration for that tooling.

This is because a upgrade to a maven coordinate in say /jetty-ee10/ shouldn't impact the versions in /jetty-ee9/.

Think of each dependency scope as a standalone git repository (and maven reactor), as that's how our project works right now.

This is accomplished by tooling configurations which specify a scanning root directory with include / exclude directories.

When the 12.1.x commit yanked the critical versions strings (eg: "1.2.3" and `"9.1.21.Final") and put it root, the scope of dependencies for the various Jetty EE# trees no longer have that string, it's in a parent pom, which is totally invisible.

All those tools see is that the responsibility of versions is not within the scope it being provided, so nothing will be seen, reported, or ugraded.

This is exactly the same behavior if you had the following ....

• A parent-project in a /repo-parent.git
  • the parent project specifies versions of dependencies
  • the parent project uses properties to define versions
• A child-project in /repo-child.git
  • uses maven with a reference to parent-project
  • declares some dependencies that are defined / declared in parent-project

Now you sic tooling at child-project to analyze dependencies, update them, etc.
No tooling with update the dependencies in child-project because those version strings are not there.
They are managed by a different project.

We already have dependencies with their version specified in the top level as a property, but used within a jetty-ee module. For example jboss.logging.version. Have the tools failed to work with that? Has that broken anything? So if there is a problem, you are not making it clear what it is. What is it that actually breaks, and why has it not broken already on the versions that are in the top level? I also don't see the issue if we ultimately break up into separate repositories, as maven is perfectly capable of fetching a parent pom from a different repository and we have done so in the past.

The example of jboss.logging.version is within the same scope as core, not the jetty-ee# trees.
The definition of the dependency is also in the top level pom, which the ee#-*-version properties do not do.

I understand that we don't want global properties that affect all the different eeX's in some circumstances. But that's not what is happening in 12.1. Each jetty-eeX module still has its top level jakarta.servlet.api.version property that is set in its sub-module pom. But rather than being set by an explicit version, it is set from a global eeX.jakarta.servlet.api.version property. This is so that our separate modules that test/demo eeX can use the same version of the APIs that we actually implement. Are you saying that the issue is that the tools can handle a version that is set as a property, but not a version that is set as a property that is set as a property? If that is the issue, then it is a bug on the tooling.

See above, this has knee-capped the ability of tooling to do updates, as they are not managed in the right place.
The <dependencyManagement><dependency> should be in the same pom as the property definition, which 12.1.x doesn't do right now.
There's no ability to update and manage dependencies with this split of property into a parent location, and the <dependency> in a child location.

I think it is a good feature that we can look in our top level pom and see:

     <ee8.jetty.servlet.api.version>4.0.6</ee8.jetty.servlet.api.version>
    <ee9.jakarta.servlet.api.version>5.0.0</ee9.jakarta.servlet.api.version>
    <ee10.jakarta.servlet.api.version>6.0.0</ee10.jakarta.servlet.api.version>
    <ee11.jakarta.servlet.api.version>6.1.0</ee11.jakarta.servlet.api.version>

Bad examples, each of those are "Jakarta EE" dependencies, not "Jetty EE" dependencies.
We should be depending on an bom import dependencyManagement in the /jetty-ee#/ against the Jakarta EE bom.
Our /jetty-demos/ shouldn't depend on "Jetty EE" (in fact, they don't depend on Jetty EE at all), but rather "Jakarta EE".

Having these in the top level makes sure we implement and then test/demo the same things when we say eeX.

The /tests/ directory was fine without it.
As most of the tests either declare the jetty-ee# dependency against ${project.version} and relies on transitive, or uses the /jetty-ee#/ bits and pieces from the jetty-home which isn't a maven dependency to worry about in this discussion.

The new /jetty-demos/ tree has NO Jetty EE dependencies.
It's all Jakarta EE dependencies, and all of them are <scope>provided</scope> so those are not even used in the dependency tree like you think.

HOWEVER.... I do question the value of having all the non-API dependencies in the top level. These are things that are private to the implementation and have no need of coordination over multiple sub-modules. So perhaps we should move the non-api dependencies back to the sub-modules..... however, I do see that a lot of other such dependencies are already defined in the top level. So a clear policy for such dependencies should be defined. But I still don't see the actual problem is we decided to declare all our dependencies in the top level, so long as we avoid name clashes.

Note that, API version is something that a human needs to explicitly manage. EVEN IF the dependabot tools are broken by having API versions in the top level, I don't see that as a problem as Dependabot should never be doing an upgrade from servlet 6.1 to servlet 6.2.

It's not just Dependabot (we are waiting on dependabot/dependabot-core#4364 before we can turn that on for Jetty 12.0.x trees), but things like Renovatebot (similar to dependabot that we are evaluating), our IDE tooling, and various maven plugins too.

Finally, in this PR, I see you are declaring some dependency version in the DependencyManagement clauses themselves. I really don't like mixing styles like that. I think all our dependency versions should be properties, even if only local or sub module ones. We should never need to go searching for a dependency version.... currently I look for a version in the properties of a pom.xml file, if I don't find it there, then I'm looking at the parent pom, not down the maybe long file at the dependency clauses themselves.

The changes in /jetty-demos/ can easily be changed to only use the Jakarta EE boms (in fact, I've already started that locally). That part was easy, especially considering that all of the dependencies in use are <scope>provided</scope>. I didn't commit that yet, as I was chasing the other faults in 12.1.x with MultiPart first. (also the reason I set this draft mode)

[gregw]

@joakime Reading what you are saying, it comes across as "you stupid idiots you've screwed everything up - yuck!". But what I think you are trying to say is "Oh I see what you've done there removing duplicate dependencies, however, because some of our tooling is not so good, then we might need to consider alternatives".

However, even if that is what you are saying, I still don't understand the issue. You say that some of the tool configurations are rooted at jetty-eeX, so any properties defined above that are invisible to it. Then let's not root those configurations at jetty-eeX. Always root them at the root and then include/exclude the modules/directories. Doing it this way will allow both the /jetty-eeX and related /jetty/demo/servlet-Y-demo modules to be in the same configuration. All the different runs of the tools can have their own shot at changing things in the top level (if that is desirable... no if it is API).

The example of a top level property that I picked jboss.logging.version is indeed used in each of the jetty-eeX/jetty-eeX-tests modules, so I still don't see how if this is a problem, then why it was not already a problem.

Maybe having jakarta EE boms is a good idea... if by that you mean having a pom that lists all the versions for API dependencies.. but isn't that what I was suggesting by just having the API properties in the top level (where we don't want tooling to change them anyway) and moving the implementation dependencies down to the individual modules (and tooling can update them). I think what was suggested is not too far away from what you are describing, but it is very hard to tell with your "it's all totally screwed" language.

In short, I absolutely want to see a solution that avoids duplication of API style dependencies versions... be that at the top level or some top level boms. I also think it OK for implementation level dependencies to be declared lower down in the module tree, but I do see many implementation dependencies already specified at the top level and the sky has not yet fallen as a result. So we do need a better policy for deciding where such dependencies go.

[olamy]

What really struggles me and makes the poms hard/confusing to read is those double interpolation

<jakarta.activation.api.version>${ee9.jakarta.activation.api.version}</jakarta.activation.api.version>
<weld.version>${ee9.weld.version}</weld.version>

we are in jetty-ee9 tree why not simply using ${ee9.jakarta.activation.api.version} and ${ee9.weld.version} and this can generate some very strange issues with maven when reading especially in jetty-home when reading those poms.

[janbartel]

What really struggles me and makes the poms hard/confusing to read is those double interpolation

<jakarta.activation.api.version>${ee9.jakarta.activation.api.version}</jakarta.activation.api.version>
<weld.version>${ee9.weld.version}</weld.version>

we are in jetty-ee9 tree why not simply using ${ee9.jakarta.activation.api.version} and ${ee9.weld.version} and this can generate some very strange issues with maven when reading especially in jetty-home when reading those poms.

I did it primarily because it was the least disturbance to the poms - no other pom needed to change other than the top level one in the jetty-eeX directory. There was some other (accidental?) benefit I seem to recall from one of the conversations with @joakime I think, but I just can't recall what it was.

[joakime]

Closing, as this solution is undesired by @gregw and @janbartel