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.

Sign up for GitHub

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

Jump to bottom

[SHRINKRES-351] enh: using maven 4 project-local dependency resolution, if available #341

Merged
merged 2 commits into from
Oct 2, 2024
Merged

[SHRINKRES-351] enh: using maven 4 project-local dependency resolution, if available #341

merged 2 commits into from
Oct 2, 2024

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented Sep 13, 2024 •
edited
Loading

Uses Maven 4 project-local repository to resolve articles, if available
Compatible with both maven 3 and maven 4
Maven 4 is not required and no dependencies are changed

Added 2 system properties to control new features (described in the README):

  • org.jboss.shrinkwrap.resolver.maven.skipCompilation: Flag to skip compilation of resolved artifacts (true/false) - default is false.
  • org.jboss.shrinkwrap.resolver.maven.disableProjectLocal: Flag to disable Maven 4 project-local repository (true/false) - default is false.

fixes #330

@lprimak lprimak marked this pull request as ready for review September 13, 2024 04:11
@lprimak lprimak mentioned this pull request Sep 13, 2024
Merged
@lprimak lprimak marked this pull request as draft September 14, 2024 23:18
@lprimak lprimak force-pushed the m4-local-resolver branch 2 times, most recently from 116461b to c91eff1 Compare September 15, 2024 05:03
@lprimak lprimak marked this pull request as ready for review September 15, 2024 05:04
@lprimak
Copy link
Contributor Author

lprimak commented Sep 15, 2024

@petrberan Ready to go!

@lprimak lprimak force-pushed the m4-local-resolver branch 2 times, most recently from db584fe to c83276c Compare September 16, 2024 02:43
@lprimak lprimak changed the title enh: using maven 4 project-local dependency resolution, if available [SHRINKRES-351] enh: using maven 4 project-local dependency resolution, if available Sep 21, 2024
@lprimak
Copy link
Contributor Author

lprimak commented Sep 23, 2024

ping :)

@petrberan
Copy link
Member

Hi @lprimak , sorry about the wait, I was preoccupied with a different issue and it's just now that I can have a look at your PRs.

I wasn't actually able to find anything in Maven 4 related to project local dependencies and its changes against Maven 3. Do you happen to have some documentation for that?

petrberan
petrberan reviewed Sep 25, 2024
View reviewed changes
...-embedded/src/main/java/org/jboss/shrinkwrap/resolver/impl/maven/embedded/FileExtractor.java
@@ -85,8 +85,9 @@ private void extractFileInDestinationDir() {
} catch (IOException e) {
System.err.println("Failed to unzip file: " + e.getMessage());
}
markerFileHandler.deleteMarkerFile();
System.out.printf("Resolver: Successfully extracted maven binaries from %s%n", fileToExtract);
if (markerFileHandler.deleteMarkerFile()) {
Copy link
Member

Choose a reason for hiding this comment

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

Even though this is just a simple print, I wouldn't make printing the line dependent on deleting the file. Especially if the file is not related to the actual extraction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because I was trying to fix flaky test that relies on this message.
Failing to delete marker file indicates a failure in the code, at least that's how I am reading this,
so actual functionality should be correct.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, found the test. Must admit, not my favorite way of checking whether the file was extracted, but I can understand this change.

Can I ask you to put this in a separate commit? As this isn't related to the project-local dependencies I'd prefer git blame to show a related commit when it comes to checking history of this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

petrberan
petrberan reviewed Sep 25, 2024
View reviewed changes
...solver/impl/maven/embedded/pom/equipped/PomEquippedEmbeddedMavenRunningAsDaemonTestCase.java
@@ -35,7 +35,7 @@ void testDaemonShouldWaitForBuildSuccess() throws TimeoutException {
.withWaitUntilOutputLineMatches(".*BUILD SUCCESS.*")
.build();

Awaitility.await("Wait till thread is not be alive").atMost(20, TimeUnit.SECONDS)
Awaitility.await("Wait till thread is not be alive").atMost(45, TimeUnit.SECONDS)
Copy link
Member

Choose a reason for hiding this comment

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

Why increasing the timeout? Did the changes in the PR resulted in longer run?

Copy link
Contributor Author

@lprimak lprimak Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Same here, I noticed flaky test in CI. Has nothing to do with the changes in this PR, just trying to fix flakiness as I see it :)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. Can I ask for a separate commit here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will do that. thanks

@lprimak
Copy link
Contributor Author

lprimak commented Sep 25, 2024

sorry about the wait

No problem at all!

Do you happen to have some documentation for that?

The only thing I could find is this: https://issues.apache.org/jira/browse/MNG-7629

@petrberan
Copy link
Member

Managed to have a look and the code looks solid, thank you @lprimak .

One thing I'd like to discuss here is the introduction of org.jboss.shrinkwrap.resolver.maven.disableProjectLocal. I understand that this is something that comes from Maven 4, which is not directly supported in this PR.

The thing that doesn't sit well with me is that this is enabled by default and this could lead to potential backward compatibility issues, as opposed if this was disabled by default and enabled once we support Maven 4.

WDYT?

@lprimak
Copy link
Contributor Author

lprimak commented Sep 26, 2024

Managed to have a look and the code looks solid

Thank you!

this is enabled by default and this could lead to potential backward compatibility issues

So there is a lot of thought and risk mitigation that went into this.
TLDR: The backwards compatibility risk is minimal and it works out-of-the-box for both maven 3 and 4.

Basically, there are two opposing forces at work here, "works out of the box" and "compatibility"
I evaluated the risk of both. The new functionality will only be invoked if there is project-local-repo directory.
This directory is only created by maven 4, so compatibility is pretty much a guarantee here.

Typically maven version is chosen by the users' environment. Both maven 3 and 4 users may work with the same project. What that means is that per-project configuration of the resolver is impractical.
This drives the decision closer to the one I made.

The only "issue" I can see if there is a mixed directory structure, and somehow project-local-repo directory of an unrelated project lies as a parent to the current maven project, and it has the same required dependency with the same version and it's somehow wrong. This is quite a remote possibility, but the alternative is resolver failing, so neither option works for the user anyway.

So, there is only a minuscule risk, and in this case I like to introduce a way to turn off new functionality, which is exactly what I did here.
Just in case I missed some obscure use case :) as we all do.

@lprimak
Copy link
Contributor Author

lprimak commented Sep 26, 2024

flaky test fixes are in a separate commit now

@petrberan
Copy link
Member

Understood, thank you for the explanation @lprimak . In the end, we can always file an issue and fix it afterwards, so I don't think there's anything preventing us from merging this.

I'll merge this and release a 3.3.2 next week if you don't mind as I'm not a fan of releasing on Friday, if that's fine :)

Do you want to pursue #340 any further or can that be closed?

@lprimak
Copy link
Contributor Author

lprimak commented Sep 27, 2024 •
edited
Loading

Thank you!
I will rebase #340 once this is merged. And then it’ll be ready to go as well.

You don't need to rush releasing this, as I would actually like both this and #340 to be merged,
so everything cna be released at once with upgraded dependencies, etc.

@petrberan
Copy link
Member

Merging, thank you @lprimak

@petrberan petrberan merged commit 3f2b406 into shrinkwrap:master Oct 2, 2024
2 checks passed
@lprimak lprimak deleted the m4-local-resolver branch October 2, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrberan petrberan petrberan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Resolver for Maven 4
2 participants
@lprimak @petrberan