Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#704: Remove WagonManager #831

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

andrzejj0
Copy link
Contributor

Removing legacy WagonManager and going forward with Transporter from https://maven.apache.org/resolver/index.html

Advantage: multiple transporter providers possible, not just the default Wagon. Also, future-proof.

it-transporter-002 is testing FileTransporter instead of the default Wagon.

@andrzejj0
Copy link
Contributor Author

@slawekjaranowski please review

Comment on lines 807 to 920
RemoteRepository repository = new RemoteRepository.Builder( serverId, null, uri.baseUri )
.build();
return transporterFactoryMap
.values()
.stream()
// highest priority first -> reversing the order of arguments:
.sorted( ( f1, f2 ) -> Float.compare( f2.getPriority(), f1.getPriority() ) )
.map( factory ->
{
try
{
return factory.newInstance( mavenSession.getRepositorySession(), repository );
}
catch ( NoTransporterException e )
{
log.warn( "No transporter possible for " + uri.baseUri + ": "
+ e.getMessage() );
return null;
}
} )
.filter( Objects::nonNull )
.map( transporter ->
{
try
{
GetTask getTask = new GetTask( uri.fileUri );
transporter.get( getTask );
return new RuleXpp3Reader().read( new StringReader( getTask.getDataString() ) );
}
catch ( Exception e )
{
log.warn( "Error while reading the rules string: " + e.getMessage() );
return null;
}
} )
.filter( Objects::nonNull )
.findFirst()
.orElse( null );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we can use Resolver Transports in such way.
Rather it is for internal use by Resolver.

In this case - we simply need download a resource from remote url.
Better will be use eg HttpClient from HttpComponents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, are you sure? If that is the case, then I'll provably need to use different transports depending on protocol (file, classpath, etc).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cstamas - can have a best knowledge about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, HttpClient5 is lightweight enough and I'd like to use the Fluent flavour of it, if you don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted a question to [email protected], but I can't see it. Do I need an invite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of something like that actually. E.g. provide several implementations per protocol (classpath, file, http/https) and use a factory method to select the matching implementation from a map.

Or use Wagon without WagonManager instead? Since Wagon basically already provides such a tnin layer, currently using HttpClient 4.5. Is Wagon going to be retired altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hboutemy Do you know if we can continue to use Wagon or is it going to be retired and we should be using e.g. HttpClient directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted a question to [email protected], but I can't see it. Do I need an invite?

You should first subscribe to list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but am not getting the confirmation link.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? should works .... after send email to: dev-subscribe at maven.apache.org

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Nov 28, 2022

Still waiting for an answer whether we can simply use Wagon (without WagonManager from maven-compat). Note that there's also a org.apache.maven.repository.legacy.WagonManager, of which I don't know whether it's going to be deprecated or not. Hence my question about Wagon in general.

For the time being, I'm still working on a version using HttpClient (largely based on Wagon), but I think I'll also create a version using Wagon only while waiting for the answer.

E.g.

mavenSession.getSettings().getProxies()
        .stream()
        .filter( Proxy::isActive )
        .filter( proxy -> ofNullable( proxy.getProtocol() )
                .map( p -> p.equals( uri.getScheme() ) )
                .orElse( true ) )
        .filter( proxy -> ofNullable( proxy.getNonProxyHosts() )
                .map( s -> s.split( "\\|" ) )
                .map( a -> Arrays.stream( a )
                        .noneMatch( s -> s.equals( uri.getHost() ) ) )
                .orElse( true ) )
        .findAny()
        .ifPresent( proxy ->
        {
            builder.setProxy( new HttpHost( proxy.getProtocol(), proxy.getHost(), proxy.getPort() ) );
            if ( !isBlank( proxy.getUsername() ) && !isBlank( proxy.getPassword() ) )
            {
                builder.setDefaultCredentialsProvider( new BasicCredentialsProvider()
                {{
                    setCredentials( new AuthScope( proxy.getHost(), proxy.getPort() ),
                            new UsernamePasswordCredentials( proxy.getUsername(),
                                    proxy.getPassword().toCharArray() ) );
                }} );
            }
        } );

but now I've realised we can simply use RepositorySystemSession.getProxySelector() etc. instead (and that also solves password encryption automatically) 😆

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Nov 29, 2022

Answer from the current Wagon maintainer is that it is definitely future-proof.
Got an implementation using Wagon ready.

By the way, I'm using RepositorySystemSession.getProxySelector() (and similar methods) instead of querying the settings directly, so that I don't need to bother about possible password encryption.

However, I'm still thinking whether not to simply employ HttpClient 5 instead. Disadvantage of such a solution would be that it would basically boil down to be a yet another reinterpretation of Wagon...

What do you think, @slawekjaranowski @slachiewicz ?

@andrzejj0
Copy link
Contributor Author

Rebased and resolved conflicts (a lot of them again). Still weighing in whether I should go with this or simply HttpClient5.

@andrzejj0
Copy link
Contributor Author

As you can see -- implementation using Wagon is ready and kicking, but I am still thinking whether not to go with pure HttpClient.

@andrzejj0 andrzejj0 marked this pull request as ready for review December 1, 2022 15:55
@andrzejj0
Copy link
Contributor Author

I've decided that it's not worth it to go with HttpClient5 directly. Too much adapting plus Wagon is already a thin layer around it. Maybe later.

@slawekjaranowski please review
if you decide it's ready, please merge

@andrzejj0 andrzejj0 marked this pull request as draft December 2, 2022 13:36
@andrzejj0 andrzejj0 marked this pull request as ready for review December 2, 2022 13:40
@slawekjaranowski slawekjaranowski merged commit f61270b into mojohaus:master Dec 2, 2022
@slawekjaranowski
Copy link
Member

Ok, good enough

@slawekjaranowski slawekjaranowski added this to the 2.14.0 milestone Dec 2, 2022
@andrzejj0 andrzejj0 deleted the remove-legacy-wagon branch December 2, 2022 17:14
@andrzejj0
Copy link
Contributor Author

Still working on a version with HttpClient5. But that will come later. Perhaps in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants