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

keeping the original ${project.basedir} by using MavenProject.setPomFile method #30

Merged
merged 2 commits into from
Sep 21, 2017

Conversation

strpbrk
Copy link
Contributor

@strpbrk strpbrk commented Feb 2, 2017

My web application build was incomplete after adding the flatten plugin into my flow.
I was using a custom configuration and putting the flattened pom file into "target/myartifact-flat.pom" ("${project.build.directory}/${project.build.finalName}-flat.pom"). Putting it there seemed logical to me, and all other projects before WAR were working correctly.

The reason it didn't work for WAR is that MavenProject.setFile() modifies the ${project.basedir}. Plugins that run after will see the wrong value, and maven-war-plugin is a plugin that uses that value.

This simple patch resolves the problem, but unfortunately it raises maven requirements to 3.2.5 (setPomFile was added in maven 3.2.4, but for some reason that version is not available).

Here is the commit that introduced this call. It seems to be added specifically to support POM editing.
apache/maven@c15226f

@hohwille
Copy link
Member

Sorry, for the late response - I was totally under water with lots of other OSS and closed projects. Thanks for your PR. The problem you are describing is exactly why I switched from writing the flattened POM to project.target directory to a "hidden" .flattened-pom.xml:
https://github.com/mojohaus/flatten-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/flatten/AbstractFlattenMojo.java#L45
That also raised the demand to create a separate clean mojo for flatten what is not required otherwise.
When I did this I was not aware of the root of the problem. Thanks for figuring this out and great that you found a fix. I would be happy to merge this and probably even change the default back to the target directory but before I would need to check some implications.
Do you happen to know if raising to 3.2.5 has any impact on requirements for the maven version installed? IMHO not any relevant but only when a stoneage version of maven is around.
If the implications are not reasonable, I am happy to merge this soon, but give me ~ one week.
@khmarbaise @rfscholte if you have any insights here, I would appreciate your wisdom.
The fix of this PR is easy and good and we should definitely merge this sooner or later. IMHO the question is if we first pull out a bugfix release 1.0.1 without this one and then get this in or if it should go to 1.0.1 (my feeling is that this is more something for an 1.1.0). I promise that it wont take yet another 6 month but have this feature out within 2017.

@cboehme
Copy link
Contributor

cboehme commented Sep 15, 2017

Regarding the requirement on Maven 3.2.5: PR #34 already raised the minimum required Maven version to 3.5.0.

@rfscholte
Copy link
Member

I'd prefer not to depend on only one version of Maven, so I have my doubts regarding the solution for #34.
3.2.5 seems okay as a minumum requirement to me, that means support for 5 official versions of Maven where 3.2.5 is almost 3 years old.

@cboehme
Copy link
Contributor

cboehme commented Sep 18, 2017

I'd also preferred not to depend on the latest Maven version only but I didn't see a solution for #34 which worked with older Maven versions due to a bug in Maven 3.3.1, 3.3.3 and 3.3.9.
If 3.5.0 as minimum requirement is not desired (which I understand) PR #34 needs to be unmerged from master branch.

@hohwille hohwille changed the base branch from master to 1.0.x September 21, 2017 19:57
@hohwille hohwille merged commit 574a080 into mojohaus:1.0.x Sep 21, 2017
@hohwille hohwille added this to the 1.0.1 milestone Sep 21, 2017
@hohwille hohwille self-assigned this Sep 21, 2017
@hohwille hohwille self-requested a review September 21, 2017 20:06
@hohwille hohwille added the bug label Sep 21, 2017
@hohwille
Copy link
Member

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

Successfully merging this pull request may close these issues.

None yet

4 participants