Gradle bump strategy #33453

ldt-fweichert · 2025-01-07T19:03:50Z

Changes

added support for the bump strategy for gradle versioning using the maven style range syntax
added support for maven style range syntax to the gradle manager

WIP!

This pr is not yet ready to merge, it does already exists for the purpose of getting feedback and input from discussions. Reviews welcome.
Todos: (for me)

Context

Documentation (please check one with an [x])

I have updated the documentation, or
No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

Code inspection only, or
Newly added/modified unit tests, or
No unit tests but ran on a real repository, or
Both unit tests + ran on a real repository

CLAassistant · 2025-01-07T19:03:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Churro

Thanks for the contribution. Looks promising but can benefit from some refinement:

Please add a reproduction repo so that your changes can be tested against, incl a dep with regular maven based range and one with a range + strict version enforcement.
Run prettier and undo changes unrelated to the bump strategy.
Keep dep extraction to the minimum, it's not the place to decide what part to bump

Most importantly: To reduce the complexity of your changes, it would be great if you could limit this PR to the already supported Maven range syntax, for now ignoring gradle's strict version notation. This should allow you to reuse the existing maven range parsing, it should work without changes to the parser and, unless I missed something, only require a few adaptations in the gradle versioning

Churro · 2025-01-09T21:56:49Z

lib/modules/manager/gradle/utils.ts

+      const updatablePart = mavenStyleRange.upperBound ?? mavenStyleRange.lowerBound;
+      return {
+        depName: `${groupId}:${artifactId}`,
+        currentValue: updatablePart,


currentValue is supposed to include the version value found for the dependency that is extracted. It's not the right place to decide whether to update the lower or upper bound. I think updating the lower bound is a bit pointless overall. Such a check can be done later while comparing update candidates with the full range (and the comparison logic in the versioning).

The only thing that may need adaptation here is the addition of ! and a space to the version regex.

Churro · 2025-01-09T22:01:20Z

lib/modules/manager/gradle/parser.ts

  for (const line of input.split(newlineRegex)) {
    const lineMatch = propRegex.exec(line);
    if (lineMatch?.groups) {
      const { key, value, leftPart } = lineMatch.groups;
+      // TODO: we dont need to check isDependency, the parseDependency does this by itself


That's correct but please keep your changes strictly limited to what's relevant for the functionality you'd like to add. Refactoring can be done separately.

Churro · 2025-01-09T22:03:56Z

lib/modules/manager/gradle/utils.ts

@@ -44,6 +53,11 @@ export function isDependencyString(input: string): boolean {
    return false;
  }

+  const mavenStyleRange = parseMavenStyleRange(tempVersionPart);
+  if (mavenStyleRange) {
+    return true;


This is wrong as it bypasses all subsequent checks

Churro · 2025-01-09T22:06:01Z

lib/modules/manager/gradle/utils.ts

@@ -44,6 +53,11 @@ export function isDependencyString(input: string): boolean {
    return false;
  }

+  const mavenStyleRange = parseMavenStyleRange(tempVersionPart);


I see no need for this here. Should be enough to have versionLikeSubstring match ranges, which it already can except for those containing ! and spaces.

Churro · 2025-01-09T22:07:24Z

lib/modules/manager/gradle/utils.ts

 );

-const versionLikeRegex = regEx('^(?<version>[-_.\\[\\](),a-zA-Z0-9+]+)');
+const versionLikeRegex = regEx('^(?<version>[-_.\\[\\](),a-zA-Z0-9+! ]*[-_.\\[\\](),a-zA-Z0-9+])');


What's the intention here? Is it an attempt to add ! and a space to the list of allowed characters? If yes, this can be done simpler

Churro · 2025-01-09T22:18:04Z

lib/modules/versioning/gradle/compare.ts

+
+const mavenStyleRangeRegex = regEx(/^(?<openingBracket>[[\](])(?<lowerBound>[-._+a-zA-Z0-9]+)?(?<seperator>\s*,\s*)(?<upperBound>[-._+a-zA-Z0-9]+)?(?<closingBracket>[[\])])(?:!!(?<preferredVersion>[-._+a-zA-Z0-9]+))?$/)
+
+export function parseMavenStyleRange(input: string): MavenStyleRange | null {


There already is mavenBasedRangeRegex and parseMavenBasedRange, which does quite the same, except that it lacks support for preferred versions. What speaks against extending the existing implementation instead of reinventing maven style range parsing as a whole?

Churro · 2025-01-09T22:19:29Z

lib/modules/versioning/gradle/index.ts

@@ -128,30 +131,31 @@ const matches = (a: string, b: string): boolean => {
    return equals(x, y);
  }

-  const mavenBasedRange = parseMavenBasedRange(b);


This looks wrong to me. It seems you're trying to replace parseMavenBasedRange() with your implementation, while it's still used e.g. in isValid().

Churro · 2025-01-09T22:46:22Z

lib/modules/versioning/gradle/index.ts

-  const mavenBasedRange = parseMavenBasedRange(b);
-  if (!mavenBasedRange) {
+  const mavenStyleRange = parseMavenStyleRange(b);
+  // we can not update an exclusive lower bound as we don't know


Do you mean (1.0,) for example? If so, this should be reflected in a test too

Churro · 2025-01-09T22:54:12Z

lib/modules/manager/gradle/utils.ts

+
+    const mavenStyleRange = parseMavenStyleRange(fullValue);
+    if (mavenStyleRange) {
+      const updatablePart = mavenStyleRange.upperBound ?? mavenStyleRange.lowerBound;


I guess ?? mavenStyleRange.lowerBound can never hapepns since parseMavenStyleRange returns null if not both lower and upper are provided?!

ldt-fweichert added 4 commits January 7, 2025 13:46

added range support to parser

e961b3e

added versioning support

2e43ff0

fixed some stuff

f1d94c9

Merge branch 'main' into gradle-version-ranges

ffa0f59

ldt-fweichert changed the title ~~Gradle version ranges~~ Gradle bump strategy Jan 7, 2025

removed unused variable

36c766c

rarkins requested a review from Churro January 8, 2025 09:19

Churro suggested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gradle bump strategy #33453

Gradle bump strategy #33453

ldt-fweichert commented Jan 7, 2025

CLAassistant commented Jan 7, 2025

Churro left a comment

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025

Churro Jan 9, 2025


		const mavenStyleRangeRegex = regEx(/^(?<openingBracket>[[\](])(?<lowerBound>[-._+a-zA-Z0-9]+)?(?<seperator>\s,\s)(?<upperBound>[-._+a-zA-Z0-9]+)?(?<closingBracket>[[\])])(?:!!(?<preferredVersion>[-._+a-zA-Z0-9]+))?$/)

		export function parseMavenStyleRange(input: string): MavenStyleRange \| null {

Gradle bump strategy #33453

Are you sure you want to change the base?

Gradle bump strategy #33453

Conversation

ldt-fweichert commented Jan 7, 2025

Changes

WIP!

Context

Documentation (please check one with an [x])

How I've tested my work (please select one)

CLAassistant commented Jan 7, 2025

Churro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment