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

hpi:run uses wrong jpi/hpl from .jenkins-hpl-map #415

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jgreffe
Copy link

@jgreffe jgreffe commented Dec 23, 2022

https://issues.jenkins.io/browse/JENKINS-70329

Check if the resolved the.hpl from /Users/xxx/.jenkins-hpl-map are contained within the current maven root project.
If not, logs a WARN instead of an INFO.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jgreffe jgreffe changed the title Jenkins 70329 hpi:run uses wrong jpi/hpl from .jenkins-hpl-map Dec 23, 2022
@NotMyFault NotMyFault added the bug label Dec 26, 2022
@@ -43,6 +43,7 @@
import org.apache.maven.project.ProjectDependenciesResolver;
import org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolver;
import org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolverException;
import org.apache.tools.ant.Project;
Copy link
Member

Choose a reason for hiding this comment

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

unsued and wrong import?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed

Comment on lines 424 to 432
private File getRootBasedir(MavenProject mavenProject) {
if (mavenProject.getParent() == null ||
mavenProject.getParent().getBasedir() == null ||
!mavenProject.getBasedir().getAbsolutePath()
.contains(mavenProject.getParent().getBasedir().getAbsolutePath())) {
return mavenProject.getBasedir();
}
return getRootBasedir(mavenProject.getParent());
}
Copy link
Member

Choose a reason for hiding this comment

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

this does not appear to be correct. There is no guarantee that a parent in of any submodule in a build is in the reactor, or parent structure.

For more reading check the difference between an aggregator project and a parent. It is possible to be both an aggregator and a parent, but it is not required.

Copy link
Member

Choose a reason for hiding this comment

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

you could look at the current maven session and what modules are in it.
or you could possibly use the maven.multiModuleProjectDirectory property.

Copy link
Author

Choose a reason for hiding this comment

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

Pushed changes

Comment on lines +26 to +35
<module>sub-module1</module>
<module>sub-module2</module>
<module>sub-module3</module>
<module>sub-module4</module>
<module>sub-module5</module>
<module>sub-module6</module>
<module>sub-module7</module>
<module>sub-module8</module>
<module>sub-module9</module>
<module>sub-module10</module>
Copy link
Member

Choose a reason for hiding this comment

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

do you really need 10 modules to validate this?

Copy link
Author

Choose a reason for hiding this comment

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

It's an arbitrary number of modules, could be less, but we may have it working by chance even without the fix.

# under the License.
#

invoker.goals=-ntp clean install hpi:run -Djetty.skip=true -DskipTests
Copy link
Member

Choose a reason for hiding this comment

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

this obviously works - but how come? hpi:run would normally run forever - does it terminate as there is no console?

Copy link
Author

Choose a reason for hiding this comment

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

Property -Djetty.skip=true avoids the jetty server to run, we don't start any infinite process.

@jgreffe jgreffe requested a review from jtnord January 11, 2023 15:02
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Not the right fix I think. Fails to diagnose the essential problem: that the *.properties file is randomly sorted, rather than chronologically.

Probably it would suffice to just invert the map, so that it is from id to file, so that mvn install on a new checkout would overwrite previous entries of the same GAV. I cannot recall why it was in the current order to begin with.

if (src.getAbsolutePath().contains(rootBasedir.getAbsolutePath())) {
getLog().info("Copying snapshot dependency Jenkins plugin " + src);
} else {
getLog().warn("Copying snapshot dependency Jenkins plugin " + src + " from outside the current root base dir " + rootBasedir);
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a warning; it is perfectly normal.

Did you just mean to use -pl to select a submodule to run?

@jglick
Copy link
Member

jglick commented Jan 27, 2023

I cannot recall why it was in the current order to begin with.

One reason I remember now: in the normal case that the snapshot version changes (in the case of core or a plugin using maven-release-plugin rather than CD), you would otherwise get an ever-growing list of entries with the same groupId / artifactId but different versions, with no garbage collection. The current scheme at least ensures that after a version change (in the same checkout directory), the stale entry is overwritten.

An alternative scheme which would address JENKINS-70329 simply while avoiding an ever-growing file might be to write out directory / GAV pairs in reverse chronological order (implies using some format other than Properties, which discards order; see also #428) and discard anything beyond the most recent 100 or 1000 or whatever.

Another solution would be to invert the map but use GA rather than GAV as the key, and include the version in the value alongside the file path. It is after all unlikely that you would switch back and forth between projects using different versions of the same snapshot dependency and expect mvn hpi:run or mvn test to work in each without reinstalling dependencies.

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.

4 participants