Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

rfscholte
Having a closer look at this commit, I actually don't like the idea that  
ModelResolver now supports versionRanges.
IMO the version should always be specific *before* resolving the model.
IIUC correctly this is required to supported version-ranges for managed  
dependencies, and that is also something I wonder if we want that.

Please let us reconsider this commit.

thanks,
Robert


On Wed, 08 Mar 2017 19:29:51 +0100, <[hidden email]> wrote:

> Repository: maven
> Updated Branches:
>   refs/heads/master 114ef6c5a -> ab800b0cf
>
>
> [MNG-6182] ModelResolver interface enhancements.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/maven/repo
> Commit: http://git-wip-us.apache.org/repos/asf/maven/commit/ab800b0c
> Tree: http://git-wip-us.apache.org/repos/asf/maven/tree/ab800b0c
> Diff: http://git-wip-us.apache.org/repos/asf/maven/diff/ab800b0c
>
> Branch: refs/heads/master
> Commit: ab800b0cfae4e3ca9453304e3b9727ba4a4b712b
> Parents: 114ef6c
> Author: Christian Schulte <[hidden email]>
> Authored: Sat Jan 30 19:17:34 2016 +0100
> Committer: Christian Schulte <[hidden email]>
> Committed: Wed Mar 8 18:24:18 2017 +0100
>
> ----------------------------------------------------------------------
>  .../maven/project/ProjectModelResolver.java     | 84 +++++++++++++++----
>  .../maven/model/resolution/ModelResolver.java   | 32 ++++++++
>  .../internal/DefaultModelResolver.java          | 85  
> ++++++++++++++++----
>  3 files changed, 167 insertions(+), 34 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/maven/blob/ab800b0c/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java
> ----------------------------------------------------------------------
> diff --git  
> a/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java  
> b/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java
> index 7b93217..3a31d33 100644
> ---  
> a/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java
> +++  
> b/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java
> @@ -28,7 +28,7 @@ import java.util.Set;
> import com.google.common.base.Predicate;
>  import com.google.common.collect.Iterables;
> -
> +import org.apache.maven.model.Dependency;
>  import org.apache.maven.model.Parent;
>  import org.apache.maven.model.Repository;
>  import org.apache.maven.model.building.FileModelSource;
> @@ -203,24 +203,26 @@ public class ProjectModelResolver
>          return new FileModelSource( pomFile );
>      }
> -    public ModelSource resolveModel( Parent parent )
> +    @Override
> +    public ModelSource resolveModel( final Parent parent )
>          throws UnresolvableModelException
>      {
> -        Artifact artifact = new DefaultArtifact( parent.getGroupId(),  
> parent.getArtifactId(), "", "pom",
> -                                                 parent.getVersion() );
> -
> -        VersionRangeRequest versionRangeRequest = new  
> VersionRangeRequest( artifact, repositories, context );
> -        versionRangeRequest.setTrace( trace );
> -
>          try
>          {
> -            VersionRangeResult versionRangeResult =  
> resolver.resolveVersionRange( session, versionRangeRequest );
> +            final Artifact artifact = new DefaultArtifact(  
> parent.getGroupId(), parent.getArtifactId(), "", "pom",
> +                                                            
> parent.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new  
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =  
> resolver.resolveVersionRange( session, versionRangeRequest );
>             if ( versionRangeResult.getHighestVersion() == null )
>              {
> -                throw new UnresolvableModelException( "No versions  
> matched the requested range '" + parent.getVersion()
> -                                                          + "'",  
> parent.getGroupId(), parent.getArtifactId(),
> -                                                      
> parent.getVersion() );
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested  
> parent version range '%s'",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),  
> parent.getVersion() );
>             }
> @@ -229,21 +231,69 @@ public class ProjectModelResolver
>                       &&  
> versionRangeResult.getVersionConstraint().getRange().getUpperBound() ==  
> null )
>              {
>                  // Message below is checked for in the MNG-2199 core IT.
> -                throw new UnresolvableModelException( "The requested  
> version range '" + parent.getVersion()
> -                                                          + "' does not  
> specify an upper bound", parent.getGroupId(),
> -                                                      
> parent.getArtifactId(), parent.getVersion() );
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested parent version range  
> '%s' does not specify an upper bound",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),  
> parent.getVersion() );
>             }
>             parent.setVersion(  
> versionRangeResult.getHighestVersion().toString() );
> +
> +            return resolveModel( parent.getGroupId(),  
> parent.getArtifactId(), parent.getVersion() );
>          }
> -        catch ( VersionRangeResolutionException e )
> +        catch ( final VersionRangeResolutionException e )
>          {
>              throw new UnresolvableModelException( e.getMessage(),  
> parent.getGroupId(), parent.getArtifactId(),
>                                                    parent.getVersion(),  
> e );
>         }
> +    }
> -        return resolveModel( parent.getGroupId(),  
> parent.getArtifactId(), parent.getVersion() );
> +    @Override
> +    public ModelSource resolveModel( final Dependency dependency )
> +        throws UnresolvableModelException
> +    {
> +        try
> +        {
> +            final Artifact artifact = new DefaultArtifact(  
> dependency.getGroupId(), dependency.getArtifactId(), "",
> +                                                           "pom",  
> dependency.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new  
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =  
> resolver.resolveVersionRange( session, versionRangeRequest );
> +
> +            if ( versionRangeResult.getHighestVersion() == null )
> +            {
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested  
> dependency version range '%s'",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(),  
> dependency.getArtifactId(), dependency.getVersion() );
> +
> +            }
> +
> +            if ( versionRangeResult.getVersionConstraint() != null
> +                     &&  
> versionRangeResult.getVersionConstraint().getRange() != null
> +                     &&  
> versionRangeResult.getVersionConstraint().getRange().getUpperBound() ==  
> null )
> +            {
> +                // Message below is checked for in the MNG-4463 core IT.
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested dependency version  
> range '%s' does not specify an upper bound",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(),  
> dependency.getArtifactId(), dependency.getVersion() );
> +
> +            }
> +
> +            dependency.setVersion(  
> versionRangeResult.getHighestVersion().toString() );
> +
> +            return resolveModel( dependency.getGroupId(),  
> dependency.getArtifactId(), dependency.getVersion() );
> +        }
> +        catch ( VersionRangeResolutionException e )
> +        {
> +            throw new UnresolvableModelException( e.getMessage(),  
> dependency.getGroupId(), dependency.getArtifactId(),
> +                                                  
> dependency.getVersion(), e );
> +
> +        }
>      }
>  }
>
> http://git-wip-us.apache.org/repos/asf/maven/blob/ab800b0c/maven-model-builder/src/main/java/org/apache/maven/model/resolution/ModelResolver.java
> ----------------------------------------------------------------------
> diff --git  
> a/maven-model-builder/src/main/java/org/apache/maven/model/resolution/ModelResolver.java  
> b/maven-model-builder/src/main/java/org/apache/maven/model/resolution/ModelResolver.java
> index c81a536..39695f7 100644
> ---  
> a/maven-model-builder/src/main/java/org/apache/maven/model/resolution/ModelResolver.java
> +++  
> b/maven-model-builder/src/main/java/org/apache/maven/model/resolution/ModelResolver.java
> @@ -19,6 +19,7 @@ package org.apache.maven.model.resolution;
>   * under the License.
>   */
> +import org.apache.maven.model.Dependency;
>  import org.apache.maven.model.Parent;
>  import org.apache.maven.model.Repository;
>  import org.apache.maven.model.building.ModelSource;
> @@ -47,16 +48,47 @@ public interface ModelResolver
>     /**
>       * Tries to resolve the POM for the specified parent coordinates  
> possibly updating {@code parent}.
> +     * <p>
> +     * Unlike the {@link #resolveModel(java.lang.String,  
> java.lang.String, java.lang.String)} method, this method
> +     * supports version ranges and updates the given {@code parent}  
> instance to match the returned {@code ModelSource}.
> +     * If {@code parent} declares a version range, the version  
> corresponding to the returned {@code ModelSource} will
> +     * be set on the given {@code parent}.
> +     * </p>
>       *
>       * @param parent The parent coordinates to resolve, must not be  
> {@code null}.
> +     *
>       * @return The source of the requested POM, never {@code null}.
> +     *
>       * @throws UnresolvableModelException If the POM could not be  
> resolved from any configured repository.
>       * @since 3.2.2
> +     *
> +     * @see Parent#clone()
>       */
>      ModelSource resolveModel( Parent parent )
>          throws UnresolvableModelException;
>     /**
> +     * Tries to resolve the POM for the specified dependency  
> coordinates possibly updating {@code dependency}.
> +     * <p>
> +     * Unlike the {@link #resolveModel(java.lang.String,  
> java.lang.String, java.lang.String)} method, this method
> +     * supports version ranges and updates the given {@code dependency}  
> instance to match the returned
> +     * {@code ModelSource}. If {@code dependency} declares a version  
> range, the version corresponding to the returned
> +     * {@code ModelSource} will be set on the given {@code dependency}.
> +     * </p>
> +     *
> +     * @param dependency The dependency coordinates to resolve, must  
> not be {@code null}.
> +     *
> +     * @return The source of the requested POM, never {@code null}.
> +     *
> +     * @throws UnresolvableModelException If the POM could not be  
> resolved from any configured repository.
> +     * @since 3.5.0-alpha-2
> +     *
> +     * @see Dependency#clone()
> +     */
> +    ModelSource resolveModel( Dependency dependency )
> +        throws UnresolvableModelException;
> +
> +    /**
>       * Adds a repository to use for subsequent resolution requests. The  
> order in which repositories are added matters,
>       * repositories that were added first should also be searched  
> first. When multiple repositories with the same
>       * identifier are added, only the first repository being added will  
> be used.
>
> http://git-wip-us.apache.org/repos/asf/maven/blob/ab800b0c/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelResolver.java
> ----------------------------------------------------------------------
> diff --git  
> a/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelResolver.java  
> b/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelResolver.java
> index 9ea91ff..3e82eb9 100644
> ---  
> a/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelResolver.java
> +++  
> b/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelResolver.java
> @@ -28,6 +28,7 @@ import java.util.Set;
> import com.google.common.base.Predicate;
>  import com.google.common.collect.Iterables;
> +import org.apache.maven.model.Dependency;
>  import org.apache.maven.model.Parent;
>  import org.apache.maven.model.Repository;
>  import org.apache.maven.model.building.FileModelSource;
> @@ -182,25 +183,27 @@ class DefaultModelResolver
>          return new FileModelSource( pomFile );
>      }
> -    public ModelSource resolveModel( Parent parent )
> +    @Override
> +    public ModelSource resolveModel( final Parent parent )
>          throws UnresolvableModelException
>      {
> -        Artifact artifact = new DefaultArtifact( parent.getGroupId(),  
> parent.getArtifactId(), "", "pom",
> -                                                 parent.getVersion() );
> -
> -        VersionRangeRequest versionRangeRequest = new  
> VersionRangeRequest( artifact, repositories, context );
> -        versionRangeRequest.setTrace( trace );
> -
>          try
>          {
> -            VersionRangeResult versionRangeResult =
> +            final Artifact artifact = new DefaultArtifact(  
> parent.getGroupId(), parent.getArtifactId(), "", "pom",
> +                                                            
> parent.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new  
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =
>                  versionRangeResolver.resolveVersionRange( session,  
> versionRangeRequest );
>             if ( versionRangeResult.getHighestVersion() == null )
>              {
> -                throw new UnresolvableModelException( "No versions  
> matched the requested range '" + parent.getVersion()
> -                                                          + "'",  
> parent.getGroupId(), parent.getArtifactId(),
> -                                                      
> parent.getVersion() );
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested  
> parent version range '%s'",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),  
> parent.getVersion() );
>             }
> @@ -209,22 +212,70 @@ class DefaultModelResolver
>                       &&  
> versionRangeResult.getVersionConstraint().getRange().getUpperBound() ==  
> null )
>              {
>                  // Message below is checked for in the MNG-2199 core IT.
> -                throw new UnresolvableModelException( "The requested  
> version range '" + parent.getVersion()
> -                                                          + "' does not  
> specify an upper bound", parent.getGroupId(),
> -                                                      
> parent.getArtifactId(), parent.getVersion() );
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested parent version range  
> '%s' does not specify an upper bound",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),  
> parent.getVersion() );
>             }
>             parent.setVersion(  
> versionRangeResult.getHighestVersion().toString() );
> +
> +            return resolveModel( parent.getGroupId(),  
> parent.getArtifactId(), parent.getVersion() );
>          }
> -        catch ( VersionRangeResolutionException e )
> +        catch ( final VersionRangeResolutionException e )
>          {
>              throw new UnresolvableModelException( e.getMessage(),  
> parent.getGroupId(), parent.getArtifactId(),
>                                                    parent.getVersion(),  
> e );
>         }
> -
> -        return resolveModel( parent.getGroupId(),  
> parent.getArtifactId(), parent.getVersion() );
>      }
> +    @Override
> +    public ModelSource resolveModel( final Dependency dependency )
> +        throws UnresolvableModelException
> +    {
> +        try
> +        {
> +            final Artifact artifact = new DefaultArtifact(  
> dependency.getGroupId(), dependency.getArtifactId(), "",
> +                                                           "pom",  
> dependency.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new  
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =
> +                versionRangeResolver.resolveVersionRange( session,  
> versionRangeRequest );
> +
> +            if ( versionRangeResult.getHighestVersion() == null )
> +            {
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested  
> dependency version range '%s'",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(),  
> dependency.getArtifactId(), dependency.getVersion() );
> +
> +            }
> +
> +            if ( versionRangeResult.getVersionConstraint() != null
> +                     &&  
> versionRangeResult.getVersionConstraint().getRange() != null
> +                     &&  
> versionRangeResult.getVersionConstraint().getRange().getUpperBound() ==  
> null )
> +            {
> +                // Message below is checked for in the MNG-4463 core IT.
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested dependency version  
> range '%s' does not specify an upper bound",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(),  
> dependency.getArtifactId(), dependency.getVersion() );
> +
> +            }
> +
> +            dependency.setVersion(  
> versionRangeResult.getHighestVersion().toString() );
> +
> +            return resolveModel( dependency.getGroupId(),  
> dependency.getArtifactId(), dependency.getVersion() );
> +        }
> +        catch ( VersionRangeResolutionException e )
> +        {
> +            throw new UnresolvableModelException( e.getMessage(),  
> dependency.getGroupId(), dependency.getArtifactId(),
> +                                                  
> dependency.getVersion(), e );
> +
> +        }
> +    }
>  }

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

Christian Schulte
Am 03/10/17 um 10:42 schrieb Robert Scholte:
> Having a closer look at this commit, I actually don't like the idea that  
> ModelResolver now supports versionRanges.
> IMO the version should always be specific *before* resolving the model.
> IIUC correctly this is required to supported version-ranges for managed  
> dependencies, and that is also something I wonder if we want that.
>
> Please let us reconsider this commit.

You do notice the parent resolution is part of that interface since
Maven 3.2.2? I just added the same logic for dependencies to support
version ranges in dependency management import declarations. Version
ranges are supported everywhere but dependency management _import_
declarations. We already agreed that this is a bug fix and this would
have been part of Maven 3.4.0, if we would not have dropped it due to
the resolver changes. MNG-4463 has been reported in JIRA 23/Nov/09
09:39! It's tiresome to discuss things committed in 2014 now. See the
'ModelResolver' interface from Maven 3.2.2. It already contains that
logic for 'Parent' model objects.

<https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commitdiff;h=045bd1503b70738ffd0fa08e336574107aded801>

The new method in 3.5.0 for 'Dependency' model objects will only be used
to support version ranges in dependency management _import_ declarations
coming in 3.5.1.

<https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=d0911ac57dccb758435cdfd3495121ec9f0ae1b4>

Regards,
--
Christian


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

rfscholte
On Fri, 10 Mar 2017 16:47:45 +0100, Christian Schulte <[hidden email]>  
wrote:

> Am 03/10/17 um 10:42 schrieb Robert Scholte:
>> Having a closer look at this commit, I actually don't like the idea that
>> ModelResolver now supports versionRanges.
>> IMO the version should always be specific *before* resolving the model.
>> IIUC correctly this is required to supported version-ranges for managed
>> dependencies, and that is also something I wonder if we want that.
>>
>> Please let us reconsider this commit.
>
> You do notice the parent resolution is part of that interface since
> Maven 3.2.2? I just added the same logic for dependencies to support
> version ranges in dependency management import declarations. Version
> ranges are supported everywhere but dependency management _import_
> declarations. We already agreed that this is a bug fix and this would
> have been part of Maven 3.4.0, if we would not have dropped it due to
> the resolver changes. MNG-4463 has been reported in JIRA 23/Nov/09
> 09:39! It's tiresome to discuss things committed in 2014 now. See the
> 'ModelResolver' interface from Maven 3.2.2. It already contains that
> logic for 'Parent' model objects.
>
> <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commitdiff;h=045bd1503b70738ffd0fa08e336574107aded801>
>
> The new method in 3.5.0 for 'Dependency' model objects will only be used
> to support version ranges in dependency management _import_ declarations
> coming in 3.5.1.
>
> <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=d0911ac57dccb758435cdfd3495121ec9f0ae1b4>
>
> Regards,

MNG-4463 is marked for 3.5.1, not for 3.5.0, so this would *not* the the  
moment to commit it.

Let's first push 3.5.0 and give everybody the change to dive into the  
complexity of dependency resolution once we start with 3.5.1

Robert

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

Christian Schulte
Am 03/10/17 um 20:16 schrieb Robert Scholte:

> On Fri, 10 Mar 2017 16:47:45 +0100, Christian Schulte <[hidden email]>  
> wrote:
>
>> Am 03/10/17 um 10:42 schrieb Robert Scholte:
>>> Having a closer look at this commit, I actually don't like the idea that
>>> ModelResolver now supports versionRanges.
>>> IMO the version should always be specific *before* resolving the model.
>>> IIUC correctly this is required to supported version-ranges for managed
>>> dependencies, and that is also something I wonder if we want that.
>>>
>>> Please let us reconsider this commit.
>>
>> You do notice the parent resolution is part of that interface since
>> Maven 3.2.2? I just added the same logic for dependencies to support
>> version ranges in dependency management import declarations. Version
>> ranges are supported everywhere but dependency management _import_
>> declarations. We already agreed that this is a bug fix and this would
>> have been part of Maven 3.4.0, if we would not have dropped it due to
>> the resolver changes. MNG-4463 has been reported in JIRA 23/Nov/09
>> 09:39! It's tiresome to discuss things committed in 2014 now. See the
>> 'ModelResolver' interface from Maven 3.2.2. It already contains that
>> logic for 'Parent' model objects.
>>
>> <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commitdiff;h=045bd1503b70738ffd0fa08e336574107aded801>
>>
>> The new method in 3.5.0 for 'Dependency' model objects will only be used
>> to support version ranges in dependency management _import_ declarations
>> coming in 3.5.1.
>>
>> <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=d0911ac57dccb758435cdfd3495121ec9f0ae1b4>
>>
>> Regards,
>
> MNG-4463 is marked for 3.5.1, not for 3.5.0, so this would *not* the the  
> moment to commit it.

Did you read the description of MNG-6182? There is a good reason not to
enhance a public API in a patch release. Just take a look at the history
of the ModelResolver interface. It changed in incompatible ways between
releases. I'd like this to be stable again in the next feature release.
That is 3.5.0 - not 3.5.1. That interface is just an adapter (the
maven-model-builder module does not depend on the resolver). I would not
want to add yet another adapter interface instead of enhancing the
existing one. It's *the* moment to commit this. That's why I added a
method not used by anything at the moment. Maven 3.5.0 still is a
drop-in replacement that way. No one needs to update an implementation.
This will change as soon as something starts to call the new method in
3.5.1, of course. Just as we add dependencies to Maven 3.0.0 in the
plugins ourselves instead of - say - Maven 3.3.9, I'd like others to be
able to depend on Maven 3.5.0 to compile against that API instead of
3.5.x. I am repeating the description of MNG-6182 already. I know the
Netbeans IDE is implementing the ModelResolver interface. No one would
expect that interface to change between 3.5.0 and 3.5.1 in a way that
you get a compile time error when compiling against 3.5.1 instead of 3.5.0.

>
> Let's first push 3.5.0 and give everybody the change to dive into the  
> complexity of dependency resolution once we start with 3.5.1

What complexity are you talking about? Someone using version ranges in a
dependency management import declaration will not do this without
intention. Maven currently fails the build with an error message. Just
like mentioned in the description of MNG-4463. No one expects a public
API to be enhanced between 3.5.0 and 3.5.1. Are we really discussing the
addition of a new method *not used anywhere*? When in doubt, we should
start a vote on this.

Regards,
--
Christian


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

Christian Schulte
Am 03/10/17 um 23:33 schrieb Christian Schulte:

> Am 03/10/17 um 20:16 schrieb Robert Scholte:
>> On Fri, 10 Mar 2017 16:47:45 +0100, Christian Schulte <[hidden email]>  
>> wrote:
>>
>>> Am 03/10/17 um 10:42 schrieb Robert Scholte:
>>>> Having a closer look at this commit, I actually don't like the idea that
>>>> ModelResolver now supports versionRanges.
>>>> IMO the version should always be specific *before* resolving the model.
>>>> IIUC correctly this is required to supported version-ranges for managed
>>>> dependencies, and that is also something I wonder if we want that.
>>>>
>>>> Please let us reconsider this commit.
>>>
>>> You do notice the parent resolution is part of that interface since
>>> Maven 3.2.2? I just added the same logic for dependencies to support
>>> version ranges in dependency management import declarations. Version
>>> ranges are supported everywhere but dependency management _import_
>>> declarations. We already agreed that this is a bug fix and this would
>>> have been part of Maven 3.4.0, if we would not have dropped it due to
>>> the resolver changes. MNG-4463 has been reported in JIRA 23/Nov/09
>>> 09:39! It's tiresome to discuss things committed in 2014 now. See the
>>> 'ModelResolver' interface from Maven 3.2.2. It already contains that
>>> logic for 'Parent' model objects.
>>>
>>> <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commitdiff;h=045bd1503b70738ffd0fa08e336574107aded801>
>>>
>>> The new method in 3.5.0 for 'Dependency' model objects will only be used
>>> to support version ranges in dependency management _import_ declarations
>>> coming in 3.5.1.
>>>
>>> <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=d0911ac57dccb758435cdfd3495121ec9f0ae1b4>
>>>
>>> Regards,
>>
>> MNG-4463 is marked for 3.5.1, not for 3.5.0, so this would *not* the the  
>> moment to commit it.
>
> Did you read the description of MNG-6182? There is a good reason not to
> enhance a public API in a patch release. Just take a look at the history
> of the ModelResolver interface. It changed in incompatible ways between
> releases. I'd like this to be stable again in the next feature release.
> That is 3.5.0 - not 3.5.1. That interface is just an adapter (the
> maven-model-builder module does not depend on the resolver). I would not
> want to add yet another adapter interface instead of enhancing the
> existing one. It's *the* moment to commit this. That's why I added a
> method not used by anything at the moment. Maven 3.5.0 still is a
> drop-in replacement that way. No one needs to update an implementation.
> This will change as soon as something starts to call the new method in
> 3.5.1, of course. Just as we add dependencies to Maven 3.0.0 in the
> plugins ourselves instead of - say - Maven 3.3.9, I'd like others to be
> able to depend on Maven 3.5.0 to compile against that API instead of
> 3.5.x. I am repeating the description of MNG-6182 already. I know the
> Netbeans IDE is implementing the ModelResolver interface. No one would
> expect that interface to change between 3.5.0 and 3.5.1 in a way that
> you get a compile time error when compiling against 3.5.1 instead of 3.5.0.
>
>>
>> Let's first push 3.5.0 and give everybody the change to dive into the  
>> complexity of dependency resolution once we start with 3.5.1
>
> What complexity are you talking about? Someone using version ranges in a
> dependency management import declaration will not do this without
> intention. Maven currently fails the build with an error message. Just
> like mentioned in the description of MNG-4463. No one expects a public
> API to be enhanced between 3.5.0 and 3.5.1. Are we really discussing the
> addition of a new method *not used anywhere*? When in doubt, we should
> start a vote on this.
>
> Regards,
>

And for the very same reason I am currently reviewing this commit

<https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=1d6af709bca616f82db79009d2ebfc8da7724569>

so that the changes to the public APIs can be part of 3.5.0 in the same
way. Add deprecations to old methods, add new methods but do not change
anything else by just not calling the new methods and keep things
unchanged. I am very sure we want to ship a stable 3.5.0 API.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: maven git commit: [MNG-6182] ModelResolver interface enhancements.

stephenconnolly
Christian you need to be proactive and sort this out...

FTR as release manager for Maven core 3.5.0 I have an easy answer to any
decisions asked of me: my answer is Out! For example, If there is a S1/S2
bug in coloured logging, then either it gets fixed promptly or it is out. I
will be brutal to get 3.5.0 shipped. If there is controversy, the easiest
way to consensus is to delay the controversy and leave it out.

More seriously if the changes post 3.5.0 mean that we don't call it 3.5.1
rather call it 3.6.0, that's fine. I will not let conflict stand in the way
of getting 3.5.0 released.

Now to the case in point: Shipping a broken api in 3.5.0 sounds like S1 for
that api change, i.e. Feature does not work at all. So I advise treading
carefully.

Version numbers are just version numbers.

-Stephen

On Fri 10 Mar 2017 at 22:47, Christian Schulte <[hidden email]> wrote:

> Am 03/10/17 um 23:33 schrieb Christian Schulte:
> > Am 03/10/17 um 20:16 schrieb Robert Scholte:
> >> On Fri, 10 Mar 2017 16:47:45 +0100, Christian Schulte <[hidden email]>
> >> wrote:
> >>
> >>> Am 03/10/17 um 10:42 schrieb Robert Scholte:
> >>>> Having a closer look at this commit, I actually don't like the idea
> that
> >>>> ModelResolver now supports versionRanges.
> >>>> IMO the version should always be specific *before* resolving the
> model.
> >>>> IIUC correctly this is required to supported version-ranges for
> managed
> >>>> dependencies, and that is also something I wonder if we want that.
> >>>>
> >>>> Please let us reconsider this commit.
> >>>
> >>> You do notice the parent resolution is part of that interface since
> >>> Maven 3.2.2? I just added the same logic for dependencies to support
> >>> version ranges in dependency management import declarations. Version
> >>> ranges are supported everywhere but dependency management _import_
> >>> declarations. We already agreed that this is a bug fix and this would
> >>> have been part of Maven 3.4.0, if we would not have dropped it due to
> >>> the resolver changes. MNG-4463 has been reported in JIRA 23/Nov/09
> >>> 09:39! It's tiresome to discuss things committed in 2014 now. See the
> >>> 'ModelResolver' interface from Maven 3.2.2. It already contains that
> >>> logic for 'Parent' model objects.
> >>>
> >>> <
> https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commitdiff;h=045bd1503b70738ffd0fa08e336574107aded801
> >
> >>>
> >>> The new method in 3.5.0 for 'Dependency' model objects will only be
> used
> >>> to support version ranges in dependency management _import_
> declarations
> >>> coming in 3.5.1.
> >>>
> >>> <
> https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=d0911ac57dccb758435cdfd3495121ec9f0ae1b4
> >
> >>>
> >>> Regards,
> >>
> >> MNG-4463 is marked for 3.5.1, not for 3.5.0, so this would *not* the the
> >> moment to commit it.
> >
> > Did you read the description of MNG-6182? There is a good reason not to
> > enhance a public API in a patch release. Just take a look at the history
> > of the ModelResolver interface. It changed in incompatible ways between
> > releases. I'd like this to be stable again in the next feature release.
> > That is 3.5.0 - not 3.5.1. That interface is just an adapter (the
> > maven-model-builder module does not depend on the resolver). I would not
> > want to add yet another adapter interface instead of enhancing the
> > existing one. It's *the* moment to commit this. That's why I added a
> > method not used by anything at the moment. Maven 3.5.0 still is a
> > drop-in replacement that way. No one needs to update an implementation.
> > This will change as soon as something starts to call the new method in
> > 3.5.1, of course. Just as we add dependencies to Maven 3.0.0 in the
> > plugins ourselves instead of - say - Maven 3.3.9, I'd like others to be
> > able to depend on Maven 3.5.0 to compile against that API instead of
> > 3.5.x. I am repeating the description of MNG-6182 already. I know the
> > Netbeans IDE is implementing the ModelResolver interface. No one would
> > expect that interface to change between 3.5.0 and 3.5.1 in a way that
> > you get a compile time error when compiling against 3.5.1 instead of
> 3.5.0.
> >
> >>
> >> Let's first push 3.5.0 and give everybody the change to dive into the
> >> complexity of dependency resolution once we start with 3.5.1
> >
> > What complexity are you talking about? Someone using version ranges in a
> > dependency management import declaration will not do this without
> > intention. Maven currently fails the build with an error message. Just
> > like mentioned in the description of MNG-4463. No one expects a public
> > API to be enhanced between 3.5.0 and 3.5.1. Are we really discussing the
> > addition of a new method *not used anywhere*? When in doubt, we should
> > start a vote on this.
> >
> > Regards,
> >
>
> And for the very same reason I am currently reviewing this commit
>
> <
> https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=1d6af709bca616f82db79009d2ebfc8da7724569
> >
>
> so that the changes to the public APIs can be part of 3.5.0 in the same
> way. Add deprecations to old methods, add new methods but do not change
> anything else by just not calling the new methods and keep things
> unchanged. I am very sure we want to ship a stable 3.5.0 API.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
> --
Sent from my phone