gnodet-bot commented on code in PR #12694:
URL: https://github.com/apache/maven/pull/12694#discussion_r4057996020
##########
impl/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/PomInlinerTransformer.java:
##########
@@ -140,28 +125,12 @@ public void
injectTransformedArtifacts(RepositorySystemSession session, MavenPro
String newVersion;
if (version != null) {
HashSet<String> usedProperties = new HashSet<>();
- Map<String, String> pomProperties = pomProperties(session);
newVersion = interpolator.interpolate(version.trim(),
property -> {
- if
(session.getConfigProperties().containsKey(property)) {
- usedProperties.add(property);
- return (String)
session.getConfigProperties().get(property);
- }
- // CI-friendly version properties (revision, sha1,
changelist) may be
- // defined in the POM's <properties> section rather
than passed via -D.
- // In that case, fall back to the project's effective
properties so the
- // installed/deployed POM gets the literal version
inlined for consumers.
- String projectValue =
project.getProperties().getProperty(property);
- if (projectValue != null) {
- usedProperties.add(property);
- // Remember this value for replacePom(), which
does not have a project ref.
- // Key by groupId:artifactId:property to avoid
cross-project collision
- // in a reactor build where multiple modules may
define the same property.
- pomProperties.put(
- project.getGroupId() + ":" +
project.getArtifactId() + ":" + property,
- projectValue);
- return projectValue;
+ if
(!session.getConfigProperties().containsKey(property)) {
+ throw new IllegalArgumentException("Cannot inline
property " + property);
Review Comment:
**[high] CI-friendly version regression — reverts the GH-13192 fix**
This squash drops the `pomProperties` fallback from
`injectTransformedArtifacts()` and the corresponding fallback in
`replacePom()`. The deleted `MavenITgh13192PomInlinerCiFriendlyPropertyTest`
was specifically testing this path: projects that define `${revision}` (or
`${sha1}`, `${changelist}`) in `<properties>` rather than via `-D` on the
command line will now fail with `IllegalArgumentException: Cannot inline
property revision` in legacy mode.
The fix (`47e80246` on master) was:
1. Fall back to `project.getProperties().getProperty(property)` when the
property is not in `session.getConfigProperties()`
2. Cache the resolved value in `pomProperties` (keyed by
`groupId:artifactId:property`) for use in `replacePom()`, which has no
`project` reference
Both the fix and the IT need to be restored. The fix is currently on master
at `47e80246` and should not have been squashed away.
If this was intentional (e.g. the feature is being moved to a separate PR),
the PR description should say so explicitly and the IT should not be deleted
here.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -836,11 +847,16 @@ private PluginAnalysisResults
analyzePluginsUsingEffectiveModels(
Path originalPomPath = entry.getKey();
try {
+ // Find the corresponding temp POM path
+ Path commonRoot = findCommonRoot(pomMap.keySet());
Review Comment:
**[medium] `findCommonRoot` called per-module iteration — O(n²) on the
reactor**
`findCommonRoot(pomMap.keySet())` is called once per module inside the `for
(Map.Entry<Path, Document> entry : pomMap.entrySet())` loop. `findCommonRoot`
iterates over all paths to compute the common ancestor, so a reactor with N
modules calls it N times — each call is O(N), making the loop O(N²).
Fix: compute once before the loop and pass down (the call site in
`createTempProjectStructure` already does this correctly).
```suggestion
Path commonRoot = findCommonRoot(pomMap.keySet());
for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
Path originalPomPath = entry.getKey();
try {
// Find the corresponding temp POM path
Path relativePath = commonRoot.relativize(originalPomPath);
Path tempPomPath = tempDir.resolve(relativePath);
```
(Move the `Path commonRoot = findCommonRoot(pomMap.keySet());` line above
the `for` loop — it was at line 851 inside the loop body. The `relativePath`
and `tempPomPath` locals stay inside the loop.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
