Feat: Align Maven 3.10.x and 4.0.x#12442
Conversation
Add support for `MAVEN_REPO_CENTRAL` env variable or `maven.repo.central` property, that is able to redefine Central URL globally. Changes: * CLI ensures that IF env var is present, if transformed into property * MavenRepositorySystem obeys property, if present * ModelInterpolator (as there is SuperPOM as well) contains the main change. It defines the property and default value, and makes sure new property is always present. SuperPOM is changed too, and now references the new property.
gnodet
left a comment
There was a problem hiding this comment.
Thank you for the work on backporting MNG-8181 to Maven 3.10.x — this is a valuable feature to have aligned across the two major versions. The overall design (Super POM interpolation via SingleResponseValueSource, property lookup in MavenRepositorySystem, env-to-property promotion in MavenCli) is sound and the approach is a reasonable adaptation of the Maven 4 mechanism for Maven 3's architecture.
However, there is a bug in MavenCli that prevents the MAVEN_REPO_CENTRAL environment variable path from working (see inline comment). The maven.repo.central property path and Super POM interpolation appear correct.
Additional observations (non-blocking):
-
LegacyRepositorySystem(maven-compat):createDefaultRemoteRepository()still uses the staticRepositorySystem.DEFAULT_REMOTE_REPO_URLand doesn't accept aMavenExecutionRequest, so it won't honormaven.repo.centralat runtime. This is probably acceptable given the compat module's deprecated status, but worth a note. -
Test coverage: The PR doesn't include new unit tests for
determineDefaultRemoteRepositoryUrl()or the env variable promotion logic. The existing IT (MavenITmng8181CentralRepoTest) only tests the-Dproperty path. A unit test for the env variable flow inMavenCli.populateProperties()would have caught thecontains()bug. -
PomConstructionTestline 1402: This test assertsRepositorySystem.DEFAULT_REMOTE_REPO_URLagainst the Super POM's repository URL. Since the Super POM now uses${maven.repo.central}and theSingleResponseValueSourceprovides the default, this should still pass — but worth confirming.
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| SystemProperties.addSystemProperties(systemProperties); | ||
|
|
||
| // one distinguished env variable: MAVEN_REPO_CENTRAL; if present, we make it into property | ||
| if (systemProperties.contains(MAVEN_REPO_CENTRAL_ENV)) { |
There was a problem hiding this comment.
🐛 Bug: Properties.contains() is inherited from Hashtable.contains(Object value) and checks whether the given string exists as a value in the map, not as a key. This means the condition will almost never be true — it would only match if some other property happened to have the literal string "env.MAVEN_REPO_CENTRAL" as its value.
The intent is to check whether the key "env.MAVEN_REPO_CENTRAL" exists, which requires containsKey():
| if (systemProperties.contains(MAVEN_REPO_CENTRAL_ENV)) { | |
| if (systemProperties.containsKey(MAVEN_REPO_CENTRAL_ENV)) { |
As written, the MAVEN_REPO_CENTRAL environment variable → maven.repo.central property promotion does not work.
Add support for
MAVEN_REPO_CENTRALenv variable ormaven.repo.centralproperty, that is able to redefine Central URL globally.Changes:
Note: this is quite trivial (and maybe hacky) change, but the goal is that Maven 3.10.x CLI behave same way in respect to mentioned env and/or property variables as Maven 4+ is. MavenCLI does the "dirty job" of lifting env variable into properties, Crux of the change is in ModelBuilder as there is also SuperPOM, that unlike with Maven 4, does contains Central stanza. This also implies, that env detection is supported ONLY if MavenCli is involved (mvn CLI), but is not respected in MIMA-like use cases, but that is IMHO acceptable.