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

feat(maven-wrapper): support wrapperVersion #28670

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(maven-wrapper): support wrapperVersion #28670

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f658102
fix(maven-wrapper): wrapperVersion instead of wrapperUrl which is the…
trystian1 Apr 25, 2024
d442e8a
fix-maven-wrapper-28668: fix version regex
trystian1 May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 73 additions & 0 deletions lib/modules/manager/maven-wrapper/extract.spec.ts
View file
Open in desktop
Expand Up @@ -2,11 +2,19 @@ import { extractPackageFile } from '.';

const onlyWrapperProperties =
'wrapperUrl=https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/0.5.6/maven-wrapper-0.5.6.jar';

const onlyWrapperVersionProperties =
'wrapperVersion=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.3.0/maven-wrapper-3.3.0.jar';
Copy link

@breun breun Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

The value of the wrapperVersion property should be just a version number, so for example: wrapperVersion=3.3.1. (wrapperVersion was introduced in Maven Wrapper 3.3.1, so using 3.3.0 in the test example might be a little confusing.)

See the real-world examples attached to this comment. For instance maven-wrapper-3.3.1-only-script has these properties:

wrapperVersion=3.3.1
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip

Copy link

@breun breun Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

The trickiest case is maven-wrapper-3.3.0-only-script, because the has wrapperUrl nor wrapperVersion in .mvn/wrapper/maven-wrapper.properties:

distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip

But maven-wrapper-3.3.0-only-script also represents the state that projects are currently in if they got updated to Maven Wrapper 3.3.0 by Renovate, because only-script is the new default installation type in Maven Wrapper 3.3.0.

I believe the Maven Wrapper version for maven-wrapper-3.3.0-only-script can only be detected by looking for the string Apache Maven Wrapper startup batch script, version 3.3.0 in mvnw (or mvnw.cmd if mvnw is not present).

Copy link

Choose a reason for hiding this comment

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

I'm currently trying to find out if Maven Wrapper provides a way to update Maven Wrapper using the existing installation type, without having to explicitly specify (and thus determine) it. If so, Renovate should update Maven Wrapper like that.

If upgrading using the existing installation type can only be done by specifying the type in the update command (-Dtype=${type}), then I plan to request a property for that too (e.g. wrapperType). But for older versions that don't explicitly communicate their installation type, Renovate might still need to detect the installation type. I suggest keeping this installation type business out of scope for this current PR. That could be added later if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does it figure out on which url it should get the maven-wrapper ? Or is that now all covered in mvn ?

Copy link

@breun breun Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

The only-script type doesn’t use the Maven Wrapper JAR. The script uses curl or wget to download Maven and calls mvn directly. See https://maven.apache.org/wrapper/#usage-with-or-without-binary-jar for more info about the different types.

Renovate doesn’t need to know how this works, right?


const onlyMavenProperties =
'distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.5.4/apache-maven-3.5.4-bin.zip';

const wrapperAndMavenProperties = `distributionUrl=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/apache-maven/3.8.4/apache-maven-3.8.4-bin.zip\nwrapperUrl=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar`;

const wrapperVersionAndMavenProperties = `distributionUrl=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/apache-maven/3.8.4/apache-maven-3.8.4-bin.zip\nwrapperVersion=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.3.0/maven-wrapper-3.3.0.jar`;
Copy link
Member

Choose a reason for hiding this comment

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

please use codeBlock template helper for better readability. use only for your new tests


const wrapperVersionAndMavenPropertiesAndWrapperUrl = `distributionUrl=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/apache-maven/3.8.4/apache-maven-3.8.4-bin.zip\nwrapperVersion=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.3.0/maven-wrapper-3.3.0.jar\nwrapperUrl=https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar`;

describe('modules/manager/maven-wrapper/extract', () => {
describe('extractPackageFile()', () => {
it('extracts version for property file with distribution type "bin" in distributionUrl', () => {
Expand Down Expand Up @@ -64,6 +72,71 @@ describe('modules/manager/maven-wrapper/extract', () => {
]);
});

it('extracts version for property file with maven wrapper version and maven version', () => {
const res = extractPackageFile(wrapperVersionAndMavenProperties);
expect(res?.deps).toEqual([
{
currentValue: '3.8.4',
replaceString:
'https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/apache-maven/3.8.4/apache-maven-3.8.4-bin.zip',
datasource: 'maven',
depName: 'maven',
packageName: 'org.apache.maven:apache-maven',
versioning: 'maven',
},
{
currentValue: '3.3.0',
replaceString:
'https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.3.0/maven-wrapper-3.3.0.jar',
datasource: 'maven',
depName: 'maven-wrapper',
packageName: 'org.apache.maven.wrapper:maven-wrapper',
versioning: 'maven',
},
]);
});

it('extracts version for property file with maven wrapper version first if both wrapperUrl and version are present', () => {
const res = extractPackageFile(
wrapperVersionAndMavenPropertiesAndWrapperUrl,
);
expect(res?.deps).toEqual([
{
currentValue: '3.8.4',
replaceString:
'https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/apache-maven/3.8.4/apache-maven-3.8.4-bin.zip',
datasource: 'maven',
depName: 'maven',
packageName: 'org.apache.maven:apache-maven',
versioning: 'maven',
},
{
currentValue: '3.3.0',
replaceString:
'https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.3.0/maven-wrapper-3.3.0.jar',
datasource: 'maven',
depName: 'maven-wrapper',
packageName: 'org.apache.maven.wrapper:maven-wrapper',
versioning: 'maven',
},
]);
});

it('extracts version for property file with maven wrapper version', () => {
const res = extractPackageFile(onlyWrapperVersionProperties);
expect(res?.deps).toEqual([
{
currentValue: '3.3.0',
replaceString:
'https://internal.artifactory.acme.org/artifactory/maven-bol/org/apache/maven/wrapper/maven-wrapper/3.3.0/maven-wrapper-3.3.0.jar',
datasource: 'maven',
depName: 'maven-wrapper',
packageName: 'org.apache.maven.wrapper:maven-wrapper',
versioning: 'maven',
},
]);
});

it('it should return null when there is no string matching the maven properties regex', () => {
const res = extractPackageFile('nowrapper');
expect(res).toBeNull();
Expand Down
9 changes: 8 additions & 1 deletion lib/modules/manager/maven-wrapper/extract.ts
View file
Open in desktop
Expand Up @@ -15,10 +15,17 @@ const WRAPPER_URL_REGEX = regEx(
'^(?:wrapperUrl\\s*=\\s*)(?<url>\\S*-(?<version>\\d+\\.\\d+(?:\\.\\d+)?(?:-\\w+)*)(?:.jar))',
);

const WRAPPER_VERSION_REGEX = regEx(
'^(?:wrapperVersion\\s*=\\s*)(?<url>\\S*-(?<version>\\d+\\.\\d+(?:\\.\\d+)?(?:-\\w+)*)(?:.jar))',
Copy link

@breun breun Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

The value of the wrapperVersion property should be just a version number, so for example: wrapperVersion=3.3.0. This shouldn't need any regular expression or other parsing. See the real-world examples attached to this comment.

Copy link
Member

Choose a reason for hiding this comment

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

it seems we only need the version group? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah so I pushed an update but that will leave the url empty,
which I think is no problem because wrapper:wrapper will do the work.

But I'm not sure how we are going to make this work (and if we should). I'm in no way a maven-wrapper expert so I will discuss with Nils if this a problem.

function getCustomMavenWrapperRepoUrl(
  deps: PackageDependency[],
): string | null {
  const replaceString = deps.find(
    (dep) => dep.depName === 'maven-wrapper',
  )?.replaceString;

  if (!replaceString) {
    return null;
  }

  const match = regEx(/^(.*?)\/org\/apache\/maven\/wrapper\//).exec(
    replaceString,
  );

  if (!match) {
    return null;
  }

  return match[1] === DEFAULT_MAVEN_REPO_URL ? null : match[1];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since it will give that as option to the maven command:

function getExtraEnvOptions(deps: PackageDependency[]): ExtraEnv {
  const customMavenWrapperUrl = getCustomMavenWrapperRepoUrl(deps);
  if (customMavenWrapperUrl) {
    return { MVNW_REPOURL: customMavenWrapperUrl };
  }
  return {};
}

);

function extractVersions(fileContent: string): MavenVersionExtract {
const lines = coerceArray(fileContent?.split(newlineRegex));
const maven = extractLineInfo(lines, DISTRIBUTION_URL_REGEX) ?? undefined;
const wrapper = extractLineInfo(lines, WRAPPER_URL_REGEX) ?? undefined;
const wrapper =
extractLineInfo(lines, WRAPPER_VERSION_REGEX) ??
extractLineInfo(lines, WRAPPER_URL_REGEX) ??
undefined;
return { maven, wrapper };
}

Expand Down