Lower-case the OS family before matching it against the family names - #13152
Conversation
The Maven 3 activator lower-cases activation/os/family before handing it to Os.isFamily (0456c7c, "Caplital OS name can not activate profile"), as it does for name and arch. The Maven 4 activator lower-cases name and arch but passes family through untouched. Os.isFamily switches on the family string against lower-case constants such as "winnt", "unix" and "dos"; anything that does not match falls through to the default branch, which is a plain actualOsName.contains(family). Families whose name is a substring of the OS name, like "Mac" on "mac os x", still work by accident, which is why the existing capital-name test did not catch this. The ones that rely on the switch do not: <family>WinNT</family> no longer activates on Windows, and <family>Mac</family> no longer activates on a JDK reporting os.name=darwin. Add the toLowerCase call so the family follows the same rule as the other three fields, and cover the two cases above.
gnodet-bot
left a comment
There was a problem hiding this comment.
The fix is correct and the root cause analysis in the description is thorough. One test coverage gap below.
Category: bug — correctness regression vs Maven 3 behaviour.
Milestone suggestion: 4.1.0 (this is a Maven 4 regression; the compat layer already has the fix).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| assertActivation(false, winnt, newContext(null, newProperties("linux", "6.5.0-1014-aws", "amd64"))); | ||
|
|
||
| Profile mac = newProfile(ActivationOS.newBuilder().family("Mac")); | ||
|
|
There was a problem hiding this comment.
Missing negation test for capitalised family.
The negation path (!WinNT) is untested. Before this fix, determineFamilyMatch("!WinNT", ...) would check "!WinNT".startsWith("!") → true, strip to "WinNT", then call Os.isFamily("WinNT", ...) → hits the default branch (substring test on lowercased form) rather than the case FAMILY_NT switch arm — so the result was wrong, and the negation then inverts the wrong result. The fix resolves this, but there is no test to pin it.
Suggest extending testCapitalFamily with a negated assertion:
| assertActivation(true, winnt, newContext(null, newProperties("windows 11", "10.0", "amd64"))); | |
| assertActivation(false, winnt, newContext(null, newProperties("linux", "6.5.0-1014-aws", "amd64"))); | |
| Profile notWinnt = newProfile(ActivationOS.newBuilder().family("!WinNT")); | |
| assertActivation(false, notWinnt, newContext(null, newProperties("windows 11", "10.0", "amd64"))); | |
| assertActivation(true, notWinnt, newContext(null, newProperties("linux", "6.5.0-1014-aws", "amd64"))); | |
| Profile mac = newProfile(ActivationOS.newBuilder().family("Mac")); | |
| assertActivation(true, mac, newContext(null, newProperties("darwin", "24.6.0", "aarch64"))); |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after second commit.
The prior finding (missing negation test for capitalised family) has been addressed in 6711e72: notWinnt cases and the mac/darwin assertion are now present and correct.
The fix itself is correct — determineFamilyMatch now matches the compat layer (maven-model-builder) byte-for-byte. Locale.ENGLISH is already imported. The !WinNT path lowers to !winnt before the startsWith("!") strip, so the lowercase is applied before negation stripping — same behaviour as name/arch/version. All test assertions verified against Os.isFamily internals (FAMILY_NT="winnt", DARWIN="darwin").
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…13152) (#13154) * Lower-case the OS family before matching it against the family names The Maven 3 activator lower-cases activation/os/family before handing it to Os.isFamily (0456c7c, "Caplital OS name can not activate profile"), as it does for name and arch. The Maven 4 activator lower-cases name and arch but passes family through untouched. Os.isFamily switches on the family string against lower-case constants such as "winnt", "unix" and "dos"; anything that does not match falls through to the default branch, which is a plain actualOsName.contains(family). Families whose name is a substring of the OS name, like "Mac" on "mac os x", still work by accident, which is why the existing capital-name test did not catch this. The ones that rely on the switch do not: <family>WinNT</family> no longer activates on Windows, and <family>Mac</family> no longer activates on a JDK reporting os.name=darwin. Add the toLowerCase call so the family follows the same rule as the other three fields, and cover the two cases above. * Add negation test for capitalised OS family --------- Co-authored-by: Leo Camus <leo.camus23@gmail.com>
I was comparing the Maven 3 profile activators in
compat/maven-model-builderwith their Maven 4 counterparts inimpl/maven-impland found one line that did not come across:OperatingSystemProfileActivator.determineFamilyMatchlower-cases the configured family in the Maven 3 copy, but not in the Maven 4 one.The Maven 3 side got that in 0456c7c ("Caplital OS name can not activate profile"), which lower-cased
name,archandfamilytogether. The Maven 4 activator introduced in d075fe7 was written from the state before that commit;nameandarchwere later brought in line,familywas not.It matters because
Os.isFamilyswitches on the family string against lower-case constants —"windows","winnt","win9x","unix","dos","mac","tandem", and so on — and only thedefaultbranch doesactualOsName.contains(family.toLowerCase(...)). So a capitalised family never reaches its own case and silently degrades into a substring test onos.name. Some families survive that by luck, because their name is a substring of the OS name:<family>Mac</family>still matchesmac os x, which is exactly the case the existingtestCapitalOsNamecovers, so the test passes while the code is wrong. The families that actually need the switch do not survive it:<family>WinNT</family>does not activate on Windows —"windows 11"does not contain"winnt".<family>Mac</family>does not activate on a JDK that reportsos.name=darwin, which is the case theDARWINconstant inOsexists for.<family>Unix</family>,<family>DOS</family>and<family>Tandem</family>are wrong the same way, and a negated form such as<family>!Unix</family>flips the wrong way, i.e. it activates a profile that was written to be excluded.Same POM, activates under Maven 3, does not under Maven 4.
The change is the single
toLowerCase(Locale.ENGLISH)call, so the family follows the same rule asname,archandversionin the same class.Localeis already imported.The new
testCapitalFamilyfails before the change and passes after:and with the one-line change applied:
I avoided
<family>Unix</family>in the test on purpose: theunixbranch ofOs.isFamilyreadsFile.pathSeparatorof the running JVM, so it would assert different things on the Linux and Windows CI runners.winntand the darwin case depend only on theos.namethe test feeds in.The whole
impl/maven-implsuite is green with the change (703 tests, 0 failures, JDK 21). I have not run the Core ITs.Your pull request should address just one issue, without pulling in other changes.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Each commit in the pull request should have a meaningful subject line and body.
Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
Run
mvn verifyto make sure basic checks pass.You have run the Core IT successfully.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5.