gnodet-bot commented on code in PR #13254:
URL: https://github.com/apache/maven/pull/13254#discussion_r4085405619
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileSelector.java:
##########
@@ -65,20 +65,32 @@ public List<Profile> getActiveProfiles(
Collection<Profile> profiles, ProfileActivationContext context,
ModelProblemCollector problems) {
List<Profile> activeProfiles = new ArrayList<>(profiles.size());
List<Profile> activePomProfilesByDefault = new ArrayList<>();
+ List<Profile> activeExternalProfilesByDefault = new ArrayList<>();
boolean activatedPomProfileNotByDefault = false;
+ // MNG-6787: track whether any profile was explicitly activated via -P
so that
+ // activeByDefault profiles from *all* sources (POM and external, e.g.
settings.xml)
+ // are correctly suppressed when the user requests a specific profile.
+ boolean anyProfileExplicitlyActivated = false;
for (Profile profile : profiles) {
if (!context.isProfileInactive(profile.getId())) {
if (context.isProfileActive(profile.getId()) ||
isActive(profile, context, problems)) {
activeProfiles.add(profile);
+ if (context.isProfileActive(profile.getId())) {
+ // Profile was explicitly requested via -P
+ anyProfileExplicitlyActivated = true;
Review Comment:
**Semantic asymmetry worth documenting.** `activatedPomProfileNotByDefault`
is set when *any* POM profile activates — whether via explicit `-P` or via a
condition activator (file, property, OS). `anyProfileExplicitlyActivated` is
set only when `context.isProfileActive()` is true (i.e., strict `-P` /
`--activate-profiles` only).
This means: if a POM profile activates via a condition (e.g.
`<activeByDefault>` suppression already happening), POM `activeByDefault`
profiles are suppressed, but external `activeByDefault` profiles are **not**.
Whether this asymmetry is intentional (per-spec: MNG-6787 says only `-P` should
suppress, not conditions) should be documented in a comment — otherwise the
next developer to read this will assume it's a bug.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileSelector.java:
##########
@@ -88,6 +100,12 @@ public List<Profile> getActiveProfiles(
activeProfiles.addAll(activePomProfilesByDefault);
}
+ // MNG-6787: only include external activeByDefault profiles when no
profile
+ // has been explicitly activated on the command line via -P.
+ if (!anyProfileExplicitlyActivated) {
+ activeProfiles.addAll(activeExternalProfilesByDefault);
Review Comment:
**Missing test coverage.** This fix has no unit test in `impl/maven-impl`.
The only `DefaultProfileSelectorTest` lives in `compat/maven-model-builder`,
which tests the *legacy* selector — not this one. Add a test directly on
`org.apache.maven.impl.model.DefaultProfileSelector` that:
1. Sets up two profiles: one external `activeByDefault`, one explicitly
active via `-P`
2. Asserts the `activeByDefault` profile is **not** returned when any
profile is explicitly requested
3. Asserts the `activeByDefault` profile **is** returned when no explicit
profile is requested
Without this, the fix has no automated regression guard.
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -462,6 +462,42 @@ if $cygwin || $mingw ; then
fi
MAVEN_PROJECTBASEDIR="$MAVEN_PROJECTBASEDIR_NATIVE"
+# MNG-8056: Under Cygwin, the JVM is a native Windows process and cannot
resolve
+# Cygwin-style POSIX paths (e.g. /cygdrive/c/...) directly. The internal paths
+# (MAVEN_HOME, CLASSWORLDS_CONF, JAVA_HOME, …) are already converted above, but
+# user-supplied paths passed via -f/--file, -s/--settings,
-gs/--global-settings, -t/--toolchains, -gt/--global-toolchains,
+# -ps/--project-settings, -is/--install-settings, -it/--install-toolchains,
+# -l/--log-file, and -af/--at-file reach the JVM unconverted.
+# The loop below rewrites only those argument values that begin with '/'
(absolute
+# POSIX paths); relative paths and already-Windows paths are left unchanged.
+# MinGW/MSYS2 perform automatic path mangling, so the fix is Cygwin-only.
+if $cygwin ; then
+ _np=false
+ _count=$#
+ while [ $_count -gt 0 ]; do
+ _a="$1"
+ shift
+ if $_np; then
+ case "$_a" in
+ /*) _a=$(cygpath --windows "$_a") ;;
+ esac
+ _np=false
+ else
+ case "$_a" in
+
-f|--file|-s|--settings|-gs|--global-settings|-t|--toolchains|-gt|--global-toolchains|-ps|--project-settings|-is|--install-settings|-it|--install-toolchains|-l|--log-file|-af|--at-file)
+ _np=true ;;
+
--file=/*|--settings=/*|--global-settings=/*|--toolchains=/*|--global-toolchains=/*|--project-settings=/*|--install-settings=/*|--install-toolchains=/*|--log-file=/*|--at-file=/*)
+ _flag="${_a%%=*}"
+ _path="${_a#*=}"
+ _a="${_flag}=$(cygpath --windows "${_path}")"
+ ;;
+ esac
+ fi
+ set -- "$@" "$_a"
+ _count=$(( _count - 1 ))
Review Comment:
⚠️ **This block is already on `master`** (merged via #13248). After rebasing
this PR onto current `master`, this entire Cygwin conversion block — along with
the corresponding changes in `test-mvn-path-conversion.sh` — will disappear
from the diff. Please rebase before requesting review, otherwise the PR appears
to contain more work than it actually does.
--
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]
