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

[MNG-8059] Paths everywhere #1413

Merged
merged 3 commits into from
Feb 26, 2024
Merged

[MNG-8059] Paths everywhere #1413

merged 3 commits into from
Feb 26, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Feb 14, 2024

Make execution flow never invoke path.toFile, at least what model-builder and maven-resolver-provider matters.


https://issues.apache.org/jira/browse/MNG-8059

@michael-o
Copy link
Member

@cstamas Seems my idea does work?

@cstamas
Copy link
Member Author

cstamas commented Feb 16, 2024

@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?

@michael-o
Copy link
Member

@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.

@cstamas
Copy link
Member Author

cstamas commented Feb 16, 2024

Hm, I think CI uses Java 11 as well? 🤷

@cstamas
Copy link
Member Author

cstamas commented Feb 17, 2024

This happened in resolver: it could allow now directory (classes in a directory) not only JAR files on "system" path:

MavenITmng3288SystemScopeDirTest.testitMNG3288:52->AbstractMavenIntegrationTestCase.fail:387 Usage of directory instead of file for system-scoped dependency did not fail dependency

This change is undone in resolver: "system" artifact can be ONLY a regular file that exists.

@cstamas cstamas marked this pull request as ready for review February 17, 2024 21:59
@cstamas cstamas marked this pull request as draft February 18, 2024 20:10
@cstamas cstamas changed the title Paths everywhere [MNG-8059] Paths everywhere Feb 20, 2024
@@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

Why actually?

Copy link
Member Author

Choose a reason for hiding this comment

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

url equality check performs hostname resolution

Copy link
Member

Choose a reason for hiding this comment

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

Really? WTF?! Why not #toAsciiString()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whatever, am just creating string out of both in same way and comparing the two.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Nice work

@cstamas
Copy link
Member Author

cstamas commented Feb 21, 2024

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.

Make execution flow never invoke path.toFile,
at least what model-builder and maven-resolver-provider
to make Resolver "paths everywhere" PR work.

Depends on Resolver 2.0.0-alpha-8 release.

---

https://issues.apache.org/jira/browse/MNG-8059
pom.xml Outdated Show resolved Hide resolved
@cstamas cstamas marked this pull request as ready for review February 26, 2024 10:33
@cstamas cstamas merged commit cf438ca into apache:master Feb 26, 2024
18 checks passed
@cstamas cstamas deleted the paths-everywhere branch February 26, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants