gnodet commented on code in PR #13401:
URL: https://github.com/apache/maven/pull/13401#discussion_r4240539222
##########
compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java:
##########
@@ -554,26 +542,9 @@ public List<String> getInactiveProfileIds() {
return delegate.getInactiveProfileIds();
}
- /**
- * System properties merged with project properties (system wins
on conflict).
- * This makes POM-declared properties visible to the
PropertyProfileActivator
- * without modifying the activator's lookup chain for non-external
models.
- * The merged map is computed once and cached to avoid repeated
allocations.
- */
@Override
public Map<String, String> getSystemProperties() {
- if (mergedSystemProperties != null) {
- return mergedSystemProperties;
- }
- Map<String, String> projectProps =
delegate.getProjectProperties();
- if (projectProps == null || projectProps.isEmpty()) {
- mergedSystemProperties =
Collections.unmodifiableMap(delegate.getSystemProperties());
- } else {
- Map<String, String> merged = new HashMap<>(projectProps);
- merged.putAll(delegate.getSystemProperties()); // system
wins
- mergedSystemProperties =
Collections.unmodifiableMap(merged);
- }
- return mergedSystemProperties;
+ return delegate.getSystemProperties();
Review Comment:
⚠️ **Compat-layer behavioral change — backport needed?**
`externalActivationContext()` previously merged POM `<properties>` into the
system-properties map so that property-based profiles in dependency POMs could
still fire. This PR removes that merging: `getSystemProperties()` now delegates
directly, stripping POM-declared properties from the sandbox entirely.
That is an intentional behaviour **reversal** in the compat layer
(`compat/maven-model-builder`). The compat layer is supposed to mirror Maven 3
semantics as closely as possible. If `maven-3.10.x` still carries the old
merging logic, this change should be backported there too — or the PR should
explicitly justify why the compat-layer behaviour intentionally diverges from
Maven 3.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1654,20 +1654,16 @@ private List<Profile> getActiveProfiles(
// A model resolved to satisfy dependency resolution -- a
dependency POM
// itself, or one of its parents, reached transitively --
evaluates profiles
// against a sandboxed activation context. The sandbox:
- // - merges model properties into system property lookups
(so POM-declared
- // <properties> drive activation via the existing
property lookup chain),
+ // - does not expose model properties through
system-property lookups, so
+ // dependency POMs cannot activate their own
property-based profiles,
// - suppresses user properties (consumer -D flags must
not activate
// dependency profiles — they were not set for that
artifact),
// - disables file existence checks (publisher paths
don't exist here).
- // Model properties are merged into system properties
rather than adding
- // a separate lookup step in PropertyProfileActivator,
because changing the
- // activator would affect ALL profile evaluations
(including the build's own
- // project) and cause unintended profile activation when a
POM declares a
- // property that matches a profile's activation condition.
// File-activated profiles are pre-filtered (not just
sandboxed) because
// returning false from exists() would incorrectly
activate <missing> profiles.
// Repository stripping is intentionally omitted: only
legitimately-active
- // profiles (JDK/OS/activeByDefault and
POM-property-gated) reach injection,
+ // profiles (JDK/OS/activeByDefault and negated-property
default-on profiles)
+ // reach injection,
Review Comment:
💬 **Minor: Awkward comment wrapping — "reach injection," dangles**
The new lines read:
```
// profiles (JDK/OS/activeByDefault and negated-property default-on profiles)
// reach injection,
// and stripping their repositories would break the established
```
`// reach injection,` is a dangling continuation, making the sentence hard
to parse. Consider collapsing the two lines:
```suggestion
// profiles (JDK/OS/activeByDefault and negated-property
default-on profiles)
// reach injection, and stripping their repositories
would break the established
```
--
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]
