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

Jump to bottom

[MENFORCER-304] - Improve dependency resolving in multiple modules project #35

Merged
merged 1 commit into from Jun 2, 2020
Merged

[MENFORCER-304] - Improve dependency resolving in multiple modules project #35

merged 1 commit into from Jun 2, 2020

Conversation

gmshake
Copy link
Contributor

@gmshake gmshake commented May 18, 2018

@gmshake gmshake mentioned this pull request May 18, 2018
Closed
@gmshake
Copy link
Contributor Author

gmshake commented Jun 27, 2018

Ping @olamy

@gmshake
Copy link
Contributor Author

gmshake commented Sep 5, 2018

Seems @olamy is busy. @khmarbaise Would you please take a look at this PR?

@khmarbaise
Copy link
Member

The first thing which I would suggest is to squash your commits into a single commit...makes it easier...

@khmarbaise khmarbaise self-requested a review September 5, 2018 19:12
@gmshake
Copy link
Contributor Author

gmshake commented Sep 6, 2018

@khmarbaise Rebased to latest master and squashed commits. :)

@gmshake
Copy link
Contributor Author

gmshake commented Nov 27, 2018

Hi @khmarbaise Any progress yet?

@mirceapopro
Copy link

mirceapopro commented Nov 20, 2019 •
edited

Hi @khmarbaise

Will you include this pull request into the next release? We have been using the maven enforcer with this fix for almost 6 month (custom build) but it would be better if the new official release has the fix.

@erilor
Copy link

erilor commented Mar 4, 2020

@khmarbaise I would also like this feature. Any chance of it getting done?

@svenlange
Copy link

Hi @khmarbaise,

could you please review and decide?

Best
Sven

@svenlange
Copy link

@elharo , could you please have a look into this PR?

@elharo elharo self-requested a review May 8, 2020 17:08
elharo
elharo reviewed May 8, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Looks OK to me. @khmarbaise do you have any concerns?

enforcer-rules/src/main/java/org/apache/maven/plugins/enforcer/AbstractBanDependencies.java Outdated
*/
public void setReactorProjects( List<MavenProject> reactorProjects )
{
this.reactorProjects = reactorProjects;
Copy link
Contributor

Choose a reason for hiding this comment

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

make a copy to avoid exposing mutable internal state.

Copy link

Choose a reason for hiding this comment

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

Suggested change
this.reactorProjects = reactorProjects;
this.reactorProjects = java.util.Collections.unmodifiableList( reactorProjects );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased the PR on latest master, and applied changes as @svenlange suggested :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@svenlange - but this will only be unmodifiable if referenced via this.reactorProjects. External code can still alter original reactorProjects that was sent to setReactorProjects method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's long time ago when I filed this PR, I can not recall why the getter and setter of reactorProjects is required. Local build looks OK without the getter and setter . :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elharo @svenlange @pzygielo
I've tested and confirmed that the getter and setter of reactorProjects are redundant. They are removed in latest commit.

Copy link

Choose a reason for hiding this comment

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

Great thanks!

Copy link

Choose a reason for hiding this comment

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

But this PR is still stuck cause @khmarbaise is not responding. Can anyone else review and decide, @elharo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elharo Friendly ping :)

@gmshake gmshake force-pushed the MENFORCER-304 branch 2 times, most recently from 8f6767a to b7162b1 Compare May 13, 2020 11:12
@gmshake gmshake changed the title [MENFORCER-304] Improve dependency resolving in multiple modules project [MENFORCER-304] - Improve dependency resolving in multiple modules project May 13, 2020
elharo
elharo reviewed May 25, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Running CI build now

@svenlange
Copy link

Running CI build now

And? :-)

@elharo
Copy link
Contributor

elharo commented Jun 2, 2020

Looks blue

https://builds.apache.org/job/maven-box/job/maven-enforcer/job/MENFORCER-304/

@elharo elharo merged commit 001396e into apache:master Jun 2, 2020
@gmshake
Copy link
Contributor Author

gmshake commented Jun 4, 2020

@elharo It's weird the master branch build fails. See https://builds.apache.org/blue/organizations/jenkins/maven-box%2Fmaven-enforcer/detail/master/135/pipeline/42/ :(

@gmshake
Copy link
Contributor Author

gmshake commented Jun 4, 2020

Local build with Oracle JDK 1.7.0_79 and maven 3.5.4 is OK.

Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-18T02:33:14+08:00)
Maven home: /Users/zlei/bin/apache-maven-3.5.4
Java version: 1.7.0_79, vendor: Oracle Corporation, runtime: /Library/Java/JavaVirtualMachines/jdk1.7.0_79.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.15.4", arch: "x86_64", family: "mac"

@gmshake
Copy link
Contributor Author

gmshake commented Jun 15, 2020 •
edited

The build is blue now :) https://builds.apache.org/job/maven-box/job/maven-enforcer/job/master/138/

@heltewig-fisp
Copy link

heltewig-fisp commented Jul 30, 2020 •
edited

I would love to see this improvement released.

Are there any plans yet for releasing 3.0.0-M4 of maven-enforcer-plugin?

@svenlange
Copy link

Is it possible to trigger a release? @elharo @khmarbaise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svenlange svenlange svenlange left review comments

@elharo elharo elharo left review comments

@pzygielo pzygielo pzygielo left review comments

@khmarbaise khmarbaise Awaiting requested review from khmarbaise

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@gmshake @khmarbaise @mirceapopro @erilor @svenlange @elharo @heltewig-fisp @pzygielo