Skip to content

[MNG-6405] Fix basedir in MavenProject.deepCopy#225

Merged
olamy merged 4 commits intoapache:masterfrom
jglick:forked-basedir-MNG-6405
Apr 16, 2019
Merged

[MNG-6405] Fix basedir in MavenProject.deepCopy#225
olamy merged 4 commits intoapache:masterfrom
jglick:forked-basedir-MNG-6405

Conversation

@jglick
Copy link
Copy Markdown
Contributor

@jglick jglick commented Jan 3, 2019

MNG-6405 since mojohaus/flatten-maven-plugin#53 (comment) brought this up again. I have not received any hints on the proper approach to test this.


By the way

  • You have run the Core IT successfully.

Is this not already done by the Jenkinsfile? Maybe an obsolete PR template.

@jglick
Copy link
Copy Markdown
Contributor Author

jglick commented Apr 12, 2019

Ping

@rfscholte
Copy link
Copy Markdown
Contributor

Looks like this is very easy to verify with a unittest, so can you add it?

@jglick
Copy link
Copy Markdown
Contributor Author

jglick commented Apr 13, 2019

I can add a unit test if you like. I am not sure that is going to be the most convincing demonstration that the reported bug is fixed, but it is something.

Copy link
Copy Markdown
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Hi, can you add the unit test as discussed above ?

@olamy
Copy link
Copy Markdown
Member

olamy commented Apr 13, 2019

IMHO it's a bit nitpicking to require test for such change...

@jglick
Copy link
Copy Markdown
Contributor Author

jglick commented Apr 15, 2019

I am looking into a test. By the way, as far as I can tell MavenProject.<init>(MavenProject) is unused, at least in this repo; the only use of deepCopy is via MavenProject.clone in MojoExecutor.executeForkedExecutions.

Failure without patch:
junit.framework.AssertionFailedError: Base directory is preserved across clone expected:<…/maven-core/target/test-classes> but was:<…/maven-core/target/test-classes/target>
	at org.apache.maven.project.MavenProjectTest.testCloneWithBaseDir(MavenProjectTest.java:188)
// copy fields
setFile( project.getFile() );
file = project.file;
basedir = project.basedir;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that these assignments are actually unnecessary when called from clone, since the fields are already set correctly by super.clone. They would however be needed if called from MavenProject.<init>(MavenProject); since that copy constructor is apparently neither called anywhere in Maven itself nor tested, I wonder if it even works.

Copy link
Copy Markdown
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

LGTM

@jira-importer
Copy link
Copy Markdown

Resolve #7456

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.

6 participants