You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/06/10 14:26:51 UTC
[GitHub] [maven-resolver] kwin opened a new pull request #53: MRESOLVER-56 make checksums configurable
kwin opened a new pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin edited a comment on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
kwin edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-643239501
Another possibility is to rely on String for this option and do the splitting afterwards like in https://github.com/apache/maven-resolver/blob/47edcfe69c4e52ced4cb93d65b7348b5645cdd68/maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/SslConfig.java#L58
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642555461
@michael-o Hopefully I addressed all your concerns in https://github.com/apache/maven-resolver/commit/347eee2fa6f0634cd459dafe3382e7ef36c87f19
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rfscholte commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642946179
First thought: add a specific property for it to `ConfigurationProperties` and make sure DefaultRepositorySystemSessionFactory sets it properly. And yes, that implies it will only work with the next Maven release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-643254535
@michael-o Please try again with https://github.com/apache/maven-resolver/pull/53/commits/1762c3df96e6c2f5901cde8c08e5f17034c415b3
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: [MRESOLVER-115] make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-646745957
Haven't received any real objections. Will go on with the merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642937562
At glance yes, but this would require you to know the semantics of each and every property. This is not possible. They are completely opaque to Maven and their meaning is only known to its consumer. If you look at `#getBoolean()` it does parse the value too. @rfscholte What is your opinion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642936252
@michael-o Shouldn't the fix rather be in https://github.com/apache/maven/blob/a2b800de32cdb9adc1e64a43a0fc32e3ba878103/maven-core/src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java#L105? IMHO parsing and splitting strings should not be done inside Maven Resolver.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #53: MRESOLVER-56 make checksums configurable
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642104183
I'd turn it into one property:
* Upload and download checksums shall be consistent
* SHA-2 family should be on by default
* It should be consistent non-brainer to say: `-Daether.checksums.algorithms=SHA-256,SHA-512,Blake2b`
This should also be a new issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642956183
>
>
> First thought: add a specific property for it to `ConfigurationProperties` and make sure DefaultRepositorySystemSessionFactory sets it properly. And yes, that implies it will only work with the next Maven release.
This feels too specific. The only cases Maven uses this class where `MavenExecutionRequest` has distinct values for this and yet they can still be overridden. We'd bascially need to get this specific value from system and user properties, process and put back again. Very, very hacky....from my POV.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642927985
How will the CLI argument `-Daether.checksums.algorithms=SHA-256` ever end up in https://github.com/apache/maven-resolver/blob/47edcfe69c4e52ced4cb93d65b7348b5645cdd68/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java#L519? I couldn't find the code setting the configProperties...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642923017
`~/apache-maven-3.7.0-SNAPSHOT/bin/mvn deploy -Daether.checksums.algorithms=SHA-256 -Dorg.slf4j.simpleLogger.log.org.apache.maven.wagon.providers.http.httpclient=debug -Dorg.slf4j.simpleLogger.log.org.apache.maven.wagon.providers.http.httpclient.wire=off` does not yet work. All checksums are uploaded.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642959446
>
>
> How will the CLI argument `-Daether.checksums.algorithms=SHA-256` ever end up in
>
> https://github.com/apache/maven-resolver/blob/47edcfe69c4e52ced4cb93d65b7348b5645cdd68/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java#L519
> ? I couldn't find the code setting the configProperties...
Via org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] kwin commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-643239501
Another option is to rely on String for this option and do the splitting afterwards like in https://github.com/apache/maven-resolver/blob/47edcfe69c4e52ced4cb93d65b7348b5645cdd68/maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/SslConfig.java#L58
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #53: [MRESOLVER-115] make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#discussion_r440103981
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -155,10 +168,12 @@ public URI getLocation( Metadata metadata, boolean upload )
private List<Checksum> getChecksums( URI location )
{
- return Arrays.asList( Checksum.forLocation( location, "SHA-512" ),
- Checksum.forLocation( location, "SHA-256" ),
- Checksum.forLocation( location, "SHA-1" ),
- Checksum.forLocation( location, "MD5" ) );
+ List<Checksum> checksums = new LinkedList<>( );
Review comment:
Agree, does this matter for a bunch of elements? Less than 10?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642929676
OK, I have found the reason. It cannot work. `org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory` sets only simple strings. `ConfigUtils.getList()` expects the object to be a collection. This means that `#getList() needs an if else for string and perform a split.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-56 make checksums configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642104183
I'd turn it into one property:
* Upload and download checksums shall be consistent
* SHA-2 family should be on by default
* It should be consistent non-brainer to say: `-Daether.checksums.algorithms=SHA-256,SHA-512,Blake2b`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] elharo commented on a change in pull request #53: [MRESOLVER-115] make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#discussion_r439874544
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/Maven2RepositoryLayoutFactory.java
##########
@@ -155,10 +168,12 @@ public URI getLocation( Metadata metadata, boolean upload )
private List<Checksum> getChecksums( URI location )
{
- return Arrays.asList( Checksum.forLocation( location, "SHA-512" ),
- Checksum.forLocation( location, "SHA-256" ),
- Checksum.forLocation( location, "SHA-1" ),
- Checksum.forLocation( location, "MD5" ) );
+ List<Checksum> checksums = new LinkedList<>( );
Review comment:
nit: arraylist is almost always better, data structures textbooks to the contrary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642929676
OK, I have found the reason. It cannot work. `org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory` sets only simple strings. `ConfigUtils.getList()` expects the object to be a collection. This means that `#getList()` needs an if else for string and perform a split.
`#getList()` is not yet used in Maven, therefore we have never noticed this. We need a new issue for this and fix it before we can merge this. Here is the spot:
```java
public static List<?> getList( Map<?, ?> properties, List<?> defaultValue, String... keys )
{
for ( String key : keys )
{
Object value = properties.get( key );
if ( value instanceof List )
{
return (List<?>) value;
}
else if ( value instanceof Collection )
{
return Collections.unmodifiableList( new ArrayList<>( (Collection<?>) value ) );
}
}
return defaultValue;
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642923017
`~/apache-maven-3.7.0-SNAPSHOT/bin/mvn deploy -Daether.checksums.algorithms=SHA-256 -Dorg.slf4j.simpleLogger.log.org.apache.maven.wagon.providers.http.httpclient=debug -Dorg.slf4j.simpleLogger.log.org.apache.maven.wagon.providers.http.httpclient.wire=off` does not yet work. All checksums are uploaded. Trying to find cause...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] asfgit closed pull request #53: [MRESOLVER-115] make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642929676
OK, I have found the reason. It cannot work. `org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory` sets only simple strings. `ConfigUtils.getList()` expects the object to be a collection. This means that `#getList()` needs an if else for string and perform a split.
`#getList()` is not yet used in Maven, therefore we have never noticed this. We need a new issue for this and fix it before we can merge this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642956183
>
>
> First thought: add a specific property for it to `ConfigurationProperties` and make sure DefaultRepositorySystemSessionFactory sets it properly. And yes, that implies it will only work with the next Maven release.
This feels to specific. The only cases Maven uses this class where `MavenExecutionRequest` has distinct values for this and yet they can still be overridden. We'd bascially need to get this specific value from system and user properties, process and put back again. Very, very hacky....from my POV.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-643598883
First upload tests look fine. I am still running Nexus 2.14.16 at work. Will try download and Core ITs. If nothing fails, I will create an upstream branch for this and leave a couple of days of review for others. When no objections are expressed, I will merge by Tue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-643643401
Branch pushed, local tests are positive.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #53: MRESOLVER-115 make checksum algorithms configurable
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #53:
URL: https://github.com/apache/maven-resolver/pull/53#issuecomment-642929676
OK, I have found the reason. It cannot work. `org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory` sets only simple strings. `ConfigUtils.getList()` expects the object to be a collection. This means that `#getList()` needs an if else for string and perform a split.
`#getList()` is not yet used in Maven, therefore we have never noticed this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org