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.

Sign up for GitHub

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

Check runtime Maven version is aligned with wrapper Maven version #36357

Merged
merged 1 commit into from Nov 21, 2023
Merged

Check runtime Maven version is aligned with wrapper Maven version #36357

merged 1 commit into from Nov 21, 2023

Conversation

jprinet
Copy link
Contributor

@jprinet jprinet commented Oct 9, 2023

Issue

When using the Maven daemon, the Maven version used can be different than the one specified by the Maven wrapper. The daemon is shipping a customized version of Maven, see here for details.
This can impact negatively the build cacheability as the Maven version is part of the cache key.

Goal of the PR

The only way to fix this issue is to have the Maven daemon using a configurable Maven version, which was pushed back in the linked ticket.
The proposal here is to tag the builds encountering this issue and add a console log to warn the user.
This will allow to identify the impacted builds.

runningcode
runningcode reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

Looks good, added some thoughts

.mvn/gradle-enterprise-custom-user-data.groovy Outdated
@@ -96,3 +96,20 @@ if (System.env.GITHUB_ACTIONS) {
}
}

// Check current Maven version and Maven Wrapper version alignment
def runtimeInfo = (org.apache.maven.rtinfo.RuntimeInformation) session.lookup("org.apache.maven.rtinfo.RuntimeInformation")
def currentMavenVersion = runtimeInfo?.getMavenVersion()
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding variable naming: instead of current since current could be used to refer to the maven daemon version or the wrapper version, can we use runtimeMavenVersion?

.mvn/gradle-enterprise-custom-user-data.groovy
mavenWrapperPropertiesFile.withInputStream {
mavenWrapperProperties.load(it)
}
if(regexp = mavenWrapperProperties."distributionUrl" =~ /.*\/apache-maven-(.*)-bin\.zip/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will work for Quarkus but we if the maven wrapper is distributed from a custom URL will this still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am parsing here a standard URL, this could not work with a custom URL indeed.
I am adding a comment to describe what is expected.

.mvn/gradle-enterprise-custom-user-data.groovy Outdated
if(regexp = mavenWrapperProperties."distributionUrl" =~ /.*\/apache-maven-(.*)-bin\.zip/) {
def wrapperMavenVersion = regexp.group(1)
if (currentMavenVersion && wrapperMavenVersion && wrapperMavenVersion != currentMavenVersion) {
log.warn("Maven Wrapper is configured with a different version (" + wrapperMavenVersion + ") than the current one (" + currentMavenVersion + "). This will negatively impact build caching.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.warn("Maven Wrapper is configured with a different version (" + wrapperMavenVersion + ") than the current one (" + currentMavenVersion + "). This will negatively impact build caching.")
log.warn("Maven Wrapper is configured with a different version (" + wrapperMavenVersion + ") than the runtime version (" + currentMavenVersion + "). This will negatively impact build consistency and build caching.")

.mvn/gradle-enterprise-custom-user-data.groovy
def wrapperMavenVersion = regexp.group(1)
if (currentMavenVersion && wrapperMavenVersion && wrapperMavenVersion != currentMavenVersion) {
log.warn("Maven Wrapper is configured with a different version (" + wrapperMavenVersion + ") than the current one (" + currentMavenVersion + "). This will negatively impact build caching.")
buildScan.tag("misaligned-maven-version")
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be valuable to add a custom value with the wrapper version? I'm assuming that the runtime version is already part of the scan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think it was necessary at first thought as the version can be obtained in the Git repo, let's add it to simplify investigation if any 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I'm not sure it is needed if it is in the repo then. It was just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it, Guillaume will decide if they want it or not

runningcode
runningcode reviewed Oct 9, 2023
View reviewed changes
.mvn/gradle-enterprise-custom-user-data.groovy Outdated
@@ -96,3 +96,23 @@ if (System.env.GITHUB_ACTIONS) {
}
}

// Check current Maven version and Maven Wrapper version alignment
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Check current Maven version and Maven Wrapper version alignment
// Check runtime Maven version and Maven Wrapper version are aligned

@jprinet jprinet marked this pull request as ready for review October 9, 2023 14:47
@jprinet jprinet changed the title Check current Maven version is aligned with wrapper Maven version Check runtime Maven version is aligned with wrapper Maven version Oct 9, 2023
@jprinet jprinet force-pushed the feature/check-maven-version-alignment branch from dec6905 to 38cb5e3 Compare October 9, 2023 14:54
@gsmet gsmet force-pushed the feature/check-maven-version-alignment branch from 3e1587b to 12cb0c4 Compare November 21, 2023 16:37
@gsmet gsmet force-pushed the feature/check-maven-version-alignment branch from 12cb0c4 to 325a1a8 Compare November 21, 2023 16:41
gsmet
gsmet approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks good to me, will wait for at least one CI run to be green before merging.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 21, 2023
@gsmet gsmet merged commit d353f20 into quarkusio:main Nov 21, 2023
43 of 49 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 21, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Nov 21, 2023
@gsmet
Copy link
Member

gsmet commented Nov 21, 2023

Thanks!

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

@runningcode runningcode runningcode approved these changes

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/maven
Projects
None yet
Milestone
3.7.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jprinet @gsmet @runningcode