You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2024/02/14 14:31:44 UTC
[PR] Paths everywhere [maven]
cstamas opened a new pull request, #1413:
URL: https://github.com/apache/maven/pull/1413
Make execution flow never invoke path.toFile,
at least what model-builder and maven-resolver-provider to make Resolver "paths everywhere" PR work.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492416418
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
Hej! Could you help me with this? To me, it seems even Maven master works with "wrong" URIs, but somehow `File` works with them (the `new File(uri)` does the job), but to me somehow it looks like we end up with File instance that explodes when `File.toPath()` is invoked on that File instance.
See this PR:
https://github.com/apache/maven/pull/1416
It passes on Win.
Then this PR, where the _only_ diff is that `new File(uri).toPath()` is invoked, and we end up with UT errors on Win like these:
https://gist.github.com/cstamas/8cf6d3e6d55f62c70f7b857effcb7bc2
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1413:
URL: https://github.com/apache/maven/pull/1413#issuecomment-1956689649
> Nice work
Thanks.
Sadly, this requires new alpha release of resolver (update + merge), so that is why would be good to have all Resolver PRs sorted out as I soon want to release it, to not have this PR rot for too long.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492501134
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
All tests pass for me on Windows.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #1413:
URL: https://github.com/apache/maven/pull/1413#issuecomment-1948442810
@cstamas Seems my idea does work?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492442692
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
Rolling...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #1413:
URL: https://github.com/apache/maven/pull/1413
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1413:
URL: https://github.com/apache/maven/pull/1413#issuecomment-1948544532
@michael-o unsure, did you change anything that is in this PR? Maybe drive letter matters? As CI keeps talking about `D:`... I mean, why does the CI fail here if all ok?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1499566180
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenSessionBuilderSupplier.java:
##########
@@ -77,7 +78,7 @@ protected DependencyTraverser getDependencyTraverser() {
}
protected DependencyManager getDependencyManager() {
- return new ClassicDependencyManager(true); // same default as in Maven4
+ return new ClassicDependencyManager(true, SystemScopeHandler.LEGACY); // same default as in Maven4
Review Comment:
This is TODO, but to me seems wrong: "same as in maven4, then legacy"?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1499567548
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/type/DefaultType.java:
##########
@@ -66,6 +66,13 @@ public DefaultType(
this.properties = Collections.unmodifiableMap(properties);
}
+ /**
+ * Copy constructor.
+ */
+ public DefaultType(String id) {
Review Comment:
TODO: this may be not needed, there is only one use of it AFAIR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1497563860
##########
maven-builder-support/src/main/java/org/apache/maven/building/UrlSource.java:
##########
@@ -87,6 +87,6 @@ public boolean equals(Object obj) {
}
UrlSource other = (UrlSource) obj;
- return this.url.equals(other.url);
+ return Objects.equals(url.toExternalForm(), other.url.toExternalForm());
Review Comment:
url equality check performs hostname resolution
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1499568014
##########
pom.xml:
##########
@@ -624,6 +624,20 @@ under the License.
<scope>test</scope>
</dependency>
</dependencies>
+
+ <repositories>
+ <repository>
+ <releases>
+ <enabled>true</enabled>
+ <updatePolicy>never</updatePolicy>
+ </releases>
+ <snapshots>
+ <enabled>false</enabled>
+ </snapshots>
+ <id>maven-2066</id>
+ <url>https://repository.apache.org/content/repositories/maven-2066/</url>
+ </repository>
+ </repositories>
Review Comment:
Remove once 2.0.0-alpha-8 released.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492421726
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
> Hej! Could you help me with this? To me, it seems even Maven master works with "wrong" URIs, but somehow `File` works with them (the `new File(uri)` does the job), but to me somehow it looks like we end up with File instance that explodes when `File.toPath()` is invoked on that File instance.
>
> See this PR: #1416
>
> It passes on Win.
>
> Then this PR, where the _only_ diff is that `new File(uri).toPath()` is invoked, and we end up with UT errors on Win like these: https://gist.github.com/cstamas/8cf6d3e6d55f62c70f7b857effcb7bc2
There are subtile conversion issues with `File#toURI()` and `Path#toUri()`. They behave differently. See https://issues.apache.org/jira/browse/SUREFIRE-2211
Try: `localRepository.toPath().toUri().toASCIIString()`. Then check whether it is absolute or that causes a problem.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492417751
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
If you are on Win, you should reproduce this just by checking out this PR, as Resolver on this PR is 'locked snapshot' so you would get the very same thing that CI builds.
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
If you are on Win, you could reproduce this just by checking out this PR, as Resolver on this PR is 'locked snapshot' so you would get the very same thing that CI builds.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1413:
URL: https://github.com/apache/maven/pull/1413#issuecomment-1948950768
Hm, I think CI uses Java 11 as well? :shrug:
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492421970
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
> If you are on Win, you could reproduce this just by checking out this PR, as Resolver on this PR is 'locked snapshot' so you would get the very same thing that CI builds.
Will try...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1497572644
##########
maven-builder-support/src/main/java/org/apache/maven/building/UrlSource.java:
##########
@@ -87,6 +87,6 @@ public boolean equals(Object obj) {
}
UrlSource other = (UrlSource) obj;
- return this.url.equals(other.url);
+ return Objects.equals(url.toExternalForm(), other.url.toExternalForm());
Review Comment:
Whatever, am just creating string out of both in same way and comparing the two.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1497569905
##########
maven-builder-support/src/main/java/org/apache/maven/building/UrlSource.java:
##########
@@ -87,6 +87,6 @@ public boolean equals(Object obj) {
}
UrlSource other = (UrlSource) obj;
- return this.url.equals(other.url);
+ return Objects.equals(url.toExternalForm(), other.url.toExternalForm());
Review Comment:
Really? WTF?! Why not `#toAsciiString()`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1497557035
##########
maven-builder-support/src/main/java/org/apache/maven/building/UrlSource.java:
##########
@@ -87,6 +87,6 @@ public boolean equals(Object obj) {
}
UrlSource other = (UrlSource) obj;
- return this.url.equals(other.url);
+ return Objects.equals(url.toExternalForm(), other.url.toExternalForm());
Review Comment:
Why actually?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1492402731
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -653,7 +653,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toURI().toASCIIString(),
Review Comment:
I hate it with a passion that Sun failed back then to make `#toString()` correctly.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Paths everywhere [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #1413:
URL: https://github.com/apache/maven/pull/1413#issuecomment-1948603718
> @michael-o unsure, did you change anything that is in this PR? Maybe drive letter matters? As CI keeps talking about `D:`... I mean, why does the CI fail here if all ok?
Didn't touch a bit. For me, your chagne runs locally with Java 11.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8059] Paths everywhere [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1413:
URL: https://github.com/apache/maven/pull/1413#discussion_r1499561548
##########
maven-core/src/main/java/org/apache/maven/bridge/MavenRepositorySystem.java:
##########
@@ -654,7 +654,7 @@ public ArtifactRepository createLocalRepository(MavenExecutionRequest request, F
public ArtifactRepository createLocalRepository(File localRepository) throws InvalidRepositoryException {
return createRepository(
- "file://" + localRepository.toURI().getRawPath(),
+ localRepository.toPath().toUri().toASCIIString(),
Review Comment:
This change seems unrelated, will try to undo this, as IMHO is actually not needed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org