maven icon indicating copy to clipboard operation
maven copied to clipboard
verified publisher icon apache

New API with immutable model

Open gnodet opened this issue 3 years ago • 20 comments

Related PRs:

  • https://github.com/apache/maven-plugin-tools/pull/117
  • https://github.com/apache/maven-plugin-testing/pull/24
  • https://github.com/apache/maven-install-plugin/pull/35
  • https://github.com/apache/maven-deploy-plugin/pull/27
  • https://github.com/apache/maven-clean-plugin/pull/20
  • https://github.com/apache/maven-compiler-plugin/pull/147
  • https://github.com/apache/maven-filtering/pull/46
  • https://github.com/apache/maven-resources-plugin/pull/35

gnodet avatar Mar 31 '22 12:03 gnodet

If we want to use immutable objects, we should use Java Records. We still do not have Valhalla ready. The developers often do not understand Java Memory Model and there is no guarantee that they would not make a mistake when they update any immutable object. The developers usually would not understand why ConcurrentLinkedQueue is preferable over ArrayList and List.copyOf() is ideal https://docs.oracle.com/javase/10/docs/api/java/util/List.html#copyOf(java.util.Collection), and so we should use List.copyOf() which means that we should switch the project to Java 10 - bad luck! Nobody would accept Java 10, I guess.

The immutable object should be fetched with data via a builder in constructor. MavenProject and MavenProjectBuilder composition

public record MavenProject(MavenProjectBuilder builder) {
 
    MavenProject {
        dependencies = List.copyOf(builder.getDependencies());
        ...
        ...
        ...
    }
}

Tibor17 avatar Apr 01 '22 08:04 Tibor17

If we want to use immutable objects, we should use Java Records. We still do not have Valhalla ready. The developers often do not understand Java Memory Model and there is no guarantee that they would not make a mistake when they update any immutable object. The developers usually would not understand why ConcurrentLinkedQueue is preferable over ArrayList and List.copyOf() is ideal https://docs.oracle.com/javase/10/docs/api/java/util/List.html#copyOf(java.util.Collection), and so we should use List.copyOf() which means that we should switch the project to Java 10 - bad luck! Nobody would accept Java 10, I guess.

The immutable object should be fetched with data via a builder in constructor. MavenProject and MavenProjectBuilder composition

public record MavenProject(MavenProjectBuilder builder) {
 
    MavenProject {
        dependencies = List.copyOf(builder.getDependencies());
        ...
        ...
        ...
    }
}

Have you had a look at the generated code or are those just general comments about java records / immutable objects ?

gnodet avatar Apr 01 '22 08:04 gnodet

Yes I saw the MDO.

Tibor17 avatar Apr 01 '22 08:04 Tibor17

Yes I saw the MDO.

So the objects from the model are immutable, they absolutely can not be modified.

So unless we raise the requirements to JDK 17, I don't see how we could actually use records / List.of(). That said, I'm not sure there's actually much difference at the end. Of course, using records would be less verbose, but given we don't have to maintain that code manually (because it's generated), I'm not sure it really matters. For the List.of(x) point, the model already uses immutable lists, though those use Collections.unmodifiableList(xxx) to be JDK 8 compatible.

gnodet avatar Apr 01 '22 08:04 gnodet

Don;t use Collections.unmodifiableList(xxx). It is vert old style - before year 2004 where the constructors where not thread safe, a typical findings with String and final fields - therefore Java changed impl for JMM of field semantics, used memory bariers at the end of constructor to make fields coherent with main mem, see JSR 133 https://download.oracle.com/otn-pub/jcp/memory_model-1.0-pfd-spec-oth-JSpec/memory_model-1_0-pfd-spec.pdf?AuthParam=1648805410_6b63e95379a8c30dbb165b2da54c9d0d

Tibor17 avatar Apr 01 '22 09:04 Tibor17

Don;t use Collections.unmodifiableList(xxx). It is vert old style - before year 2004 where the constructors where not thread safe, a typical findings with String and final fields - therefore Java changed impl for JMM of field semantics, used memory bariers at the end of constructor to make fields coherent with main mem, see JSR 133 https://download.oracle.com/otn-pub/jcp/memory_model-1.0-pfd-spec-oth-JSpec/memory_model-1_0-pfd-spec.pdf?AuthParam=1648805410_6b63e95379a8c30dbb165b2da54c9d0d

Sorry, I don't get the point of immutable collections vs memory semantics. I agree we should use List.of(xxx) if we were to raise to JDK 17, but I'm not sure there's a consensus there. What do you propose I use instead for immutable collections ? Also, in case you missed the point, I'm always using

list != null ? Collections.unmodifiableList( new ArrayList<>( list ) ) : Collections.emptyList()

in order to make sure the collections are actually safe and immutable. I can re-implement a custom unmodifiable collection to avoid the double creation, but that's more an optimisation to me rather than an actual problem.

gnodet avatar Apr 01 '22 10:04 gnodet

First question... Do we want to have a cycles between packages and interfaces? eg: org.apache.maven.api. Session needs org.apache.maven.api.services.ArtifactDeployer and ArtifactDeployer needs Session.

slawekjaranowski avatar Apr 11 '22 15:04 slawekjaranowski

First question... Do we want to have a cycles between packages and interfaces? eg: org.apache.maven.api. Session needs org.apache.maven.api.services.ArtifactDeployer and ArtifactDeployer needs Session.

I can remove all the methods from the session. Those are shortcuts to improve the user-friendliness of the api. My thinking was that simple usages would just use the Session rather than having to go through the services, for example creating an Artifact, but maybe simple usages are not really so frequent...

Or we could remove the default implementations delegating to the various services from the Session interface. The Service interface could be moved up one package (along the Session). That should allow getting rid of this cycle I think.

gnodet avatar Apr 11 '22 16:04 gnodet

First question... Do we want to have a cycles between packages and interfaces? eg: org.apache.maven.api. Session needs org.apache.maven.api.services.ArtifactDeployer and ArtifactDeployer needs Session.

The cycle has been removed.

gnodet avatar Apr 13 '22 08:04 gnodet

My wish is that for a future Maven release we have maven-xx-api and maven-xx-spi modules (I'm not sure if we need a single huge api module, or multiple small ones) , so plugins and extensions don't need to compile with maven-core. With this we can give them a module name, so developers that want to use a module descriptor can do so. However, that implies no split packages! Keep that in mind when choosing the package names.

rfscholte avatar Apr 13 '22 08:04 rfscholte

IMHO also the API from https://maven.apache.org/shared/maven-shared-utils/apidocs/org/apache/maven/shared/utils/logging/package-summary.html should be at least partially contained in the API package. Otherwise you always depend on the huge maven-shared-utils library if you want to style the output of a mojo.

kwin avatar Jun 08 '22 14:06 kwin

What about https://github.com/apache/maven/blob/2926f033cb934e7bcef51362639663bc2a39cf9a/maven-core/src/main/java/org/apache/maven/artifact/handler/DefaultArtifactHandler.java and https://github.com/apache/maven/blob/896c707d324330d7d4ad92674187923945efcda9/maven-core/src/main/java/org/apache/maven/lifecycle/mapping/DefaultLifecycleMapping.java and/or https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/lifecycle/providers/packaging/AbstractLifecycleMappingProvider.java. IMHO those are the recommended (and only?) way to override the default lifecycles for certain types (https://books.sonatype.com/mvnref-book/reference/writing-plugins-sect-plugins-lifecycle.html). Are those supposed to be implementation details or should those be moved to API as well (compare with https://issues.apache.org/jira/browse/MNG-5697).

kwin avatar Jul 04 '22 06:07 kwin

Is this change planned for Maven 4 only or will be also soon backported to 3.9 line?

slachiewicz avatar Jul 04 '22 07:07 slachiewicz

Is this change planned for Maven 4 only or will be also soon backported to 3.9 line?

No, that's definitely a longer term goal, so maven 4 only. It would still be experimental for a couple of months / releases, the time to make sure the API is complete and stable. A transition would also be needed to switch maven-core internal to this API and reverse the current code, i.e. have the old api implemented on top of the new one, so that we can drop it completely after a period.

gnodet avatar Jul 04 '22 07:07 gnodet

@gnodet Is there already a branch for any of the core plugins, to see if this API is complete? (i.e. allows to drop all other Maven dependencies)? Which plugin would be a good candidate for it?

kwin avatar Jul 04 '22 07:07 kwin

@gnodet Is there already a branch for any of the core plugins, to see if this API is complete? (i.e. allows to drop all other Maven dependencies)? Which plugin would be a good candidate for it?

Yes, I created a mvn4 branch for each maven project, but not everything has been migrated yet. https://github.com/apache/maven-plugin-tools/commits/mvn4 https://github.com/apache/maven-install-plugin/commits/mvn4 https://github.com/apache/maven-deploy-plugin/commits/mvn4 ...

gnodet avatar Jul 04 '22 08:07 gnodet

What about https://github.com/apache/maven/blob/2926f033cb934e7bcef51362639663bc2a39cf9a/maven-core/src/main/java/org/apache/maven/artifact/handler/DefaultArtifactHandler.java and https://github.com/apache/maven/blob/896c707d324330d7d4ad92674187923945efcda9/maven-core/src/main/java/org/apache/maven/lifecycle/mapping/DefaultLifecycleMapping.java and/or https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/lifecycle/providers/packaging/AbstractLifecycleMappingProvider.java. IMHO those are the recommended (and only?) way to override the default lifecycles for certain types (https://books.sonatype.com/mvnref-book/reference/writing-plugins-sect-plugins-lifecycle.html). Are those supposed to be implementation details or should those be moved to API as well (compare with https://issues.apache.org/jira/browse/MNG-5697).

The extensibility part has not been really well defined yet. I think the idea is to be really opened including whatever changes are needed for some time, but the reason I'd like to merge this PR is to be able to actually get started on other PRs, because as long as the API is not available, we can't create any PR on other projects to leverage it.

On the ArtifactHandler part, I don't think the ArtifactHandler should be visible in the API. It has to be part of the SPI which is not really defined yet.

On a related note, I think we should think whether it would make sense to split the Artifact interface into Coordinates, AttachedArtifact and ResolvedArtifact or whatever makes sense, because the Artifact interface conveys different semantics depending where it is used.

gnodet avatar Jul 04 '22 11:07 gnodet

All tests are passing. I'd like to go on and merge this PR (and release an alpha version if possible). This would allow further work on other components and plugins (see the various PRs in this PR description) which are blocked because the API is not available in any public release or snapshot version.

gnodet avatar Sep 05 '22 13:09 gnodet

👍 for merge and for release alpha

slawekjaranowski avatar Sep 05 '22 14:09 slawekjaranowski

:+1: as well, will take a deeper look tonight... but that should not block anything

cstamas avatar Sep 05 '22 14:09 cstamas

Consider using Nonnull by default, so the only place when you need to document nullability becomes @Nullable.

For instance, jspecify did not include @NonNull for a long time, and now they say @NonNull is rarely needed: https://github.com/jspecify/jspecify/blob/fa7b1d8654e5ec0696b5a8a8c49e275a2121aafd/src/main/java/org/jspecify/nullness/NonNull.java

vlsi avatar Sep 28 '22 11:09 vlsi