[MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services by gnodet · Pull Request #819 · apache/maven

gnodet · 2022-10-09T18:36:22Z

Adds ToolchainsBuilder and SettingsBuilder services.

…escriptorCreator

michael-o · 2022-10-09T18:37:49Z

Can you elaborate on the change? When was it broken?

gnodet · 2022-10-09T21:00:41Z

Can you elaborate on the change? When was it broken?

When the v4 api was merged, a few services were migrated to the new API, but that was a mistake because they are in use by plugins. This is the case for the ToolchainsBuilder and the MojoDescriptorCreator as reported on https://lists.apache.org/list.html?dev@maven.apache.org. Whether those are considered internal or part of the real api could be discussed, but they were broken, and now is not the time to break compatibility.
This PR then restores those services and add 2 new services to the new API to build settings and toolchains.

I need to investigate why the IT is failing.

michael-o

I see two general issues/questions here:

We have already briefly discussed the benefit/overrating of Optional. Can you explain why it makes sense here? I mean X != null is as good as here.
I have a general problem with the terms XBuilderRequest/XBuilderResult/XBuilderException. I think this is incorrectly names since it implies that there is something with the builder and not the build itself. In other spots we use the terms XBuildingRequest/XBuildingResult/XBuildingException/XBuildingProblem or just Build. @elharo Sample: https://github.com/apache/maven/blob/35b93b0a589752cc88105623a2ddf9e52b56c1ce/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java

michael-o · 2022-10-10T20:03:52Z

     */
    @Nonnull
-    Collection<ProjectBuilderProblem> getProblems();
+    Collection<BuilderProblem> getProblems();


Stupid question: Is it really a problem with the builder or a problem with the build?

michael-o · 2022-10-10T20:04:49Z

+     * @throws SettingsBuilderException If the effective settings could not be built.
+     */
+    @Nonnull
+    SettingsBuilderResult build( @Nonnull SettingsBuilderRequest request );


Same question here.

michael-o · 2022-10-10T20:05:33Z

+     * @return The global settings path or {@code null} if none.
+     */
+    @Nonnull
+    Optional<Path> getGlobalSettingsPath();


Javadoc does not match code

michael-o · 2022-10-10T20:05:39Z

+     * @return The global settings source or {@code null} if none.
+     */
+    @Nonnull
+    Optional<Source> getGlobalSettingsSource();


Javadoc does not match code

michael-o · 2022-10-10T20:05:44Z

+     * @return The user settings path or {@code null} if none.
+     */
+    @Nonnull
+    Optional<Path> getUserSettingsPath();


Javadoc does not match code

michael-o · 2022-10-10T20:07:49Z

+     * @return The user Toolchains path or {@code null} if none.
+     */
+    @Nonnull
+    Optional<Path> getUserToolchainsPath();


Javadoc does not match code

michael-o · 2022-10-10T20:07:54Z

+     * @return The user Toolchains source or {@code null} if none.
+     */
+    @Nonnull
+    Optional<Source> getUserToolchainsSource();


Javadoc does not match code

michael-o · 2022-10-10T20:08:00Z

+                .globalToolchainsSource( nonNull( globalToolchainsSource, "globalToolchainsSource can not be null" ) )
+                .userToolchainsSource( nonNull( userToolchainsSource, "userToolchainsSource can not be null" ) )
+                .build();
+    }


michael-o · 2022-10-10T20:08:05Z

+                .globalToolchainsPath( nonNull( globalToolchainsPath, "globalToolchainsPath can not be null" ) )
+                .userToolchainsPath( nonNull( userToolchainsPath, "userToolchainsPath can not be null" ) )
+                .build();
+    }


michael-o · 2022-10-10T20:09:33Z


                        @Override
-                        public ProjectBuilderProblemSeverity getSeverity()
+                        public BuilderProblemSeverity getSeverity()


Why isn't it BuilderProblem.Severity?

No reason, I'll move it as an inner enum.

elharo

I haven't read the whole thin yet, but I tend to agree that Optional is at best as bad as null, and sometimes worse.

elharo · 2022-10-10T21:54:25Z


+    /**
+     * Gets the user properties to use for interpolation. The user properties have been configured directly by the user
+     * on his discretion, e.g. via the {@code -Dkey=value} parameter on the command line.


delete "on his discretion"
, --> ;

elharo · 2022-10-10T21:54:43Z


+    /**
+     * Gets the system properties to use for interpolation. The system properties are collected from the runtime
+     * environment like {@link System#getProperties()} and environment variables.


like --> such as

elharo · 2022-10-10T21:55:30Z

    @Nonnull
    SessionData getData();

+    /**


Javadoc additions to existing API are useful but can be included in a separate PR that is easier to approve.

elharo · 2022-10-10T21:56:11Z

 */
 @Experimental
-public enum ProjectBuilderProblemSeverity
+public enum BuilderProblemSeverity


rename seems unnecessary

elharo · 2022-10-10T21:56:25Z

     * @param session The {@link Session}, must not be {@code null}.
-     * @param source The {@link ProjectBuilderSource}, must not be {@code null}.
-     * @throws ProjectBuilderException if the project cannot be created
+     * @param source The {@link Source}, must not be {@code null}.


The --> the

elharo · 2022-10-10T21:56:33Z

-     * @param source The {@link ProjectBuilderSource}, must not be {@code null}.
-     * @throws ProjectBuilderException if the project cannot be created
+     * @param source The {@link Source}, must not be {@code null}.
+     * @throws ProjectBuilderException if the project can not be created


revert, cannot is one word

elharo · 2022-10-10T21:57:19Z

+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.


what is "effective settings"? How does that differ from other settings?

I think "effective" here means an aggregation/merge of the global and user settings.

what is "effective settings"? How does that differ from other settings?

You need to put this in the docs, perhaps rewriting without the word effective

elharo · 2022-10-10T21:57:37Z

+     *
+     * @param request The settings building request that holds the parameters, must not be {@code null}.
+     * @return The result of the settings building, never {@code null}.
+     * @throws SettingsBuilderException If the effective settings could not be built.


if the effective settings could not be built

elharo · 2022-10-10T21:58:11Z

+    /**
+     * Builds the effective settings of the specified settings files.
+     *
+     * @param request The settings building request that holds the parameters, must not be {@code null}.


The --> the (per Oracle, API doc params do not begin with a capital letter

elharo · 2022-10-10T21:58:54Z

+{
+    /**
+     * @param message The message to give.
+     * @param e The {@link Exception}.


don't repeat type information in doc comments. It's redundant. Instead explain what this exception is

gnodet · 2022-10-11T06:33:19Z

I'm going to split that PR in 2. This one will be kept for the modifications on the v4 api : the new services and the related discussions.
I'll create a separate PR for the restoration of the compatibility.

michael-o

Clean and simple, except for the comments of @elharo

gnodet · 2022-10-11T08:00:11Z

I see two general issues/questions here:
I would rather have had those discussions when I asked for feedback on the API, but I suppose it's not too late ;-)
See my answers / reasoning below.

We have already briefly discussed the benefit/overrating of Optional. Can you explain why it makes sense here? I mean X != null is as good as here.

I slightly disagree. The main differences are that when using an Optional, the code carries the semantics and actually forces the caller to deal with it, whereas just if you simply return a value (the optionals are only used as return values), you have to get to the definition, read the javadoc, so that you can know if you need to deal with a possible null value. There are very often places in our code where you see both null checks and not and people end up adding null checks, just in case.

This also hurts code concision. A simple example found in the code:

        if ( getBaseVersionInternal() != null )
        {
            sb.append( getBaseVersionInternal() );
        }
        else
        {
            sb.append( versionRange.toString() );
        }

This could be rewritten as:

       sb.append( getBaseVersionInternal().orElse( versionRange.toString() ) );

This is actually related to the fact that the maven coding style is very sparse. I actually find that quite difficult to work with (even if I'm getting used to it). That may be due to the fact I'm working almost exclusively on a laptop, so the height of it is limited, and only seing 30-40 lines of code at the same time becomes a problem when nearly 50% of those don't really convey any semantic (being blank lines or braces usually). It means more time to scroll through the code back and forth to understand it. I don't like that. Streams are another way to get around empty lines, just because often, you simply don't need braces when using them, so you have 1 line instead of 3.

So generally speaking, the more semantic carried by the code, the better. I agree to not overuse Optional, I just don't think they are at this point. That said, the use of Nullable / NonNull annotations does help with semantics already, and I would not be opposed to removing them on requests / results objects.

I have a general problem with the terms XBuilderRequest/XBuilderResult/XBuilderException. I think this is incorrectly names since it implies that there is something with the builder and not the build itself. In other spots we use the terms XBuildingRequest/XBuildingResult/XBuildingException/XBuildingProblem or just Build. @elharo Sample: https://github.com/apache/maven/blob/35b93b0a589752cc88105623a2ddf9e52b56c1ce/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java

I'm actually seeing the terms as a request to the X builder, the result returned by X builder, the exception thrown by X builder. We can't use quotes, but think about those as XBuilder's request, XBuilder's result, XBuilder's exception. I actually think those make more sense than what we have in other places. It's also easier to see the relationship between the service and its related request/result/exception.
Also, if you think each service usually throws its own exception. If we move the semantic to what action is performed instead of who does the action, the exceptions would have to be more shared between services. However (and that's still missing on some right now), a bunch of exception are used to carry the context in which the exception was thrown.

Another point that just came to my mind is that wording like DependencyCollectionRequest is not intuitive either: the term Collection in java is immediately associated to a... java.util.Collection, and not to the act of collecting...

michael-o · 2022-10-11T12:36:01Z

I see two general issues/questions here:
I would rather have had those discussions when I asked for feedback on the API, but I suppose it's not too late ;-)
See my answers / reasoning below.

We have already briefly discussed the benefit/overrating of Optional. Can you explain why it makes sense here? I mean X != null is as good as here.

I slightly disagree. The main differences are that when using an Optional, the code carries the semantics and actually forces the caller to deal with it, whereas just if you simply return a value (the optionals are only used as return values), you have to get to the definition, read the javadoc, so that you can know if you need to deal with a possible null value. There are very often places in our code where you see both null checks and not and people end up adding null checks, just in case.

This also hurts code concision. A simple example found in the code:
        if ( getBaseVersionInternal() != null )
        {
            sb.append( getBaseVersionInternal() );
        }
        else
        {
            sb.append( versionRange.toString() );
        }
This could be rewritten as:
       sb.append( getBaseVersionInternal().orElse( versionRange.toString() ) );
This is actually related to the fact that the maven coding style is very sparse. I actually find that quite difficult to work with (even if I'm getting used to it). That may be due to the fact I'm working almost exclusively on a laptop, so the height of it is limited, and only seing 30-40 lines of code at the same time becomes a problem when nearly 50% of those don't really convey any semantic (being blank lines or braces usually). It means more time to scroll through the code back and forth to understand it. I don't like that. Streams are another way to get around empty lines, just because often, you simply don't need braces when using them, so you have 1 line instead of 3.

So generally speaking, the more semantic carried by the code, the better. I agree to not overuse Optional, I just don't think they are at this point. That said, the use of Nullable / NonNull annotations does help with semantics already, and I would not be opposed to removing them on requests / results objects.

This reasoning I can follow where Optional adds value. I don't like like to proclaim Optional to be the new null which is non sense. No objections here.

I have a general problem with the terms XBuilderRequest/XBuilderResult/XBuilderException. I think this is incorrectly names since it implies that there is something with the builder and not the build itself. In other spots we use the terms XBuildingRequest/XBuildingResult/XBuildingException/XBuildingProblem or just Build. @elharo Sample: https://github.com/apache/maven/blob/35b93b0a589752cc88105623a2ddf9e52b56c1ce/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java

I'm actually seeing the terms as a request to the X builder, the result returned by X builder, the exception thrown by X builder. We can't use quotes, but think about those as XBuilder's request, XBuilder's result, XBuilder's exception. I actually think those make more sense than what we have in other places. It's also easier to see the relationship between the service and its related request/result/exception. Also, if you think each service usually throws its own exception. If we move the semantic to what action is performed instead of who does the action, the exceptions would have to be more shared between services. However (and that's still missing on some right now), a bunch of exception are used to carry the context in which the exception was thrown.

Another point that just came to my mind is that wording like DependencyCollectionRequest is not intuitive either: the term Collection in java is immediately associated to a... java.util.Collection, and not to the act of collecting...

Let me crunch on this. @elharo What is your opinion on the style here?

gnodet · 2022-10-25T20:40:50Z

This PR has been split with #820, #852, #853, #854 for ease of review.

jira-importer · 2025-07-06T15:51:53Z

Resolve #8332

Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoD…

a59be5a

…escriptorCreator

michael-o self-requested a review October 9, 2022 18:37

Fix IT

b463ccb

cstamas approved these changes Oct 10, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into compatibility

1dc9489

gnodet changed the title ~~Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator~~ [MNG-7553] Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator Oct 10, 2022

michael-o requested changes Oct 10, 2022

View reviewed changes

elharo reviewed Oct 10, 2022

View reviewed changes

gnodet mentioned this pull request Oct 11, 2022

Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator #820

Merged

Merge remote-tracking branch 'origin/master' into compatibility

7e590a2

gnodet changed the title ~~[MNG-7553] Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator~~ [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services Oct 11, 2022

michael-o approved these changes Oct 11, 2022

View reviewed changes

Revert changes not related to the api modifications

714a9db

gnodet force-pushed the compatibility branch from 04ad5f5 to 714a9db Compare October 11, 2022 08:38

gnodet added 8 commits October 11, 2022 10:45

Fix typo "can not" -> "cannot"

2f71b47

Fix javadoc comments (first letter in lowercase and no dot at the end)

d51613c

Fix MojoException

537c5f3

Move Severity as in inner enum to BuilderProblem

505e5e7

Fix IT

1127fe4

Improve javadoc

41a3947

Remove double spaces that have been introduced

43d24f3

Add a missing @nonnull annotation

83a1cde

michael-o self-requested a review October 13, 2022 11:56

gnodet closed this Oct 25, 2022

Conversation

gnodet commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o commented Oct 9, 2022

Uh oh!

gnodet commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o commented Oct 11, 2022

Uh oh!

gnodet commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jira-importer commented Jul 6, 2025

Uh oh!

Reviewers

gnodet commented Oct 9, 2022 •

edited

Loading

gnodet commented Oct 9, 2022 •

edited

Loading

michael-o left a comment •

edited

Loading

gnodet commented Oct 11, 2022 •

edited

Loading

michael-o left a comment •

edited

Loading

gnodet commented Oct 11, 2022 •

edited

Loading

gnodet commented Oct 25, 2022 •

edited

Loading