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

Fix #324 Remove unnecessary manual dependency resolution #325

Merged
merged 1 commit into from Apr 22, 2019

Conversation

@srdo
Copy link
Contributor

@srdo srdo commented Apr 20, 2019

Please see #324

The fix here is to remove the manual dependency resolution in DependenciesTool. We already configure the mojos to require dependency resolution up to test scope, so all dependencies are already resolved and present in the injected MavenProject(s).

I realize that #219 calls for programmatic dependency resolution, but I think that will be easier to implement once the plugin is upgraded to newer APIs. For now, the code in DependenciesTool is unnecessary and causes this bug.

I'll put up a PR for upgrading to Maven 3 APIs soon, which will depend on this PR.

@srdo
Copy link
Contributor Author

@srdo srdo commented Apr 20, 2019

One of the builds on Appveyor failed, but it's not the test I added, and it doesn't fail for me locally or on Travis. The log on Appveyor just refers to the integration test build log. Do you have access to that log @ppalaga? I don't seem to be able to find it.

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 21, 2019

One of the builds on Appveyor failed, but it's not the test I added, and it doesn't fail for me locally or on Travis.

Appveyor is Windows. Do you use Windows locally?

The log on Appveyor just refers to the integration test build log. Do you have access to that log @ppalaga?

No, Appveyor, much like TravisCI has no notion of persistent workspace. The only way to debug this is to add type path\to\the\log to on_failure of appveyor.yml.

I pressed "rebuild" on the failed task, FWIW.

@srdo
Copy link
Contributor Author

@srdo srdo commented Apr 22, 2019

I use Windows, but through the Linux subsystem. It looks like the test passes now, maybe it is flaky on WIndows. I'll try running it a couple of times outside the Linux subsystem.

Regarding Appveyor, maybe we could store the target/it directory as artifacts on failure? https://help.appveyor.com/discussions/suggestions/828-store-some-artifacts-in-case-of-failure

@srdo
Copy link
Contributor Author

@srdo srdo commented Apr 22, 2019

Ran the tests 5 times locally on Windows with no failures. Maybe it was a network issue while downloading a license?

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 22, 2019

Maybe it was a network issue while downloading a license?

Yeah, that's my theory. Let me review this PR.

maybe we could store the target/it directory as artifacts on failure? https://help.appveyor.com/discussions/suggestions/828-store-some-artifacts-in-case-of-failure

A PR is welcome.

Copy link
Contributor

@ppalaga ppalaga left a comment

The changes look good at the first sight. Could you please reword the commit to Fix #324 Remove unnecessary manual dependency resolution so that the issue gets auto-closed upon merge?

Let me test with WildFly Camel now.

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 22, 2019

Does this commit have any potential to break the backwards compatibility?

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 22, 2019

Found no issue with WildFly Camel. This PR will be good to merge once you please reword to Fix #324 Remove unnecessary manual dependency resolution

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 22, 2019

My quick tests show this as a substantial perf gain on a project as large as WildFly Camel. I am getting build time 2:08 using this PR vs. 3:15 using version 1.20.

@srdo
Copy link
Contributor Author

@srdo srdo commented Apr 22, 2019

Thanks for reviewing.

Reworded commit message.

I wouldn't expect backward compatibility to be broken. The deleted code wasn't doing anything useful, I think, and any difference to the dependencies resolved by Maven was probably incorrect.

I'll take a look at the appveyor thing soon.

@ppalaga ppalaga changed the title ISSUE-324: Remove unnecessary manual dependency resolution Fix #324 Remove unnecessary manual dependency resolution Apr 22, 2019
@ppalaga ppalaga merged commit 4eff254 into mojohaus:master Apr 22, 2019
2 checks passed
@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 22, 2019

Thanks @srdo !

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

Successfully merging this pull request may close these issues.

None yet

2 participants