[MNG-8768] Add executable() function for conditional profile activation based on PATH#12332
[MNG-8768] Add executable() function for conditional profile activation based on PATH#12332Hiteshsai007 wants to merge 2 commits into
Conversation
…on based on PATH Implements MNG-8768 (GitHub apache#10115): adds an executable(name_or_path) function to Maven's condition-based profile activation DSL. The function evaluates to true if the given executable name can be found in the system PATH, or if an absolute/relative path points directly to an executable file. Use-case (from the issue): auto-detect the presence of x86_64-linux-musl-gcc when building GraalVM native images with the native-maven-plugin, and conditionally add --static --libc=musl to the build options only when the compiler wrapper is available on the current machine. Example pom.xml usage: <activation> <condition>executable('x86_64-linux-musl-gcc')</condition> </activation> Implementation details: - ConditionFunctions.executable() delegates to new ExecutableFinder helper - ExecutableFinder splits the env.PATH system property and probes each directory; on Windows it also tries .exe / .cmd / .bat / .com extensions - Absolute/relative paths (containing a path separator) are checked directly without PATH traversal - PATH is sourced from ProfileActivationContext.getSystemProperty('env.PATH') (Maven's normalised env var key), with a System.getenv('PATH') fallback Tests: - 5 new end-to-end tests in ConditionProfileActivatorTest (MNG-8768) - 12 new unit tests in ExecutableFinderTest covering PATH search, Windows extension probing, absolute paths, non-executable files, and empty PATH
gnodet
left a comment
There was a problem hiding this comment.
Fully automatic review from Claude Code
Thanks for the contribution — the use case (auto-detecting x86_64-linux-musl-gcc for GraalVM native images) is real and well-motivated. However, I have significant concerns about the current approach, primarily around build reproducibility.
Core concern: environment-dependent profile activation in published POMs
The executable() function makes profile activation depend on the runtime PATH, which is inherently non-reproducible. When a POM containing <condition>executable('x86_64-linux-musl-gcc')</condition> is published to a repository and consumed by other projects, the effective model silently changes depending on what's installed on each consumer's machine.
Today, Maven does not strip <condition> expressions from consumer POMs — they survive as-is into published artifacts (verified by reading DefaultConsumerPomBuilder). This means:
- Two developers building the same project get different dependency trees depending on what's on their
PATH - CI and local builds silently diverge
- There's no audit trail — no logging tells you "profile X activated because executable Y was found at Z"
While existing activation mechanisms (<os>, <jdk>, <property>) are also environment-dependent, they operate on a small set of well-defined, relatively stable values that flow through ProfileActivationContext. executable() is qualitatively different — PATH is per-session, per-user, per-shell, and varies wildly across machines.
Additional technical issues
1. System.getenv("PATH") fallback bypasses the activation context
In ExecutableFinder.getPathValue() (line 130), there's a System.getenv("PATH") fallback when env.PATH isn't in the context. This is the only place in the profile activation code that reaches directly into the OS environment, breaking the abstraction that all other activators respect. Every other activator uses context.getSystemProperty() exclusively.
2. System.getProperty("os.name") bypasses the context for OS detection
ExecutableFinder.isWindows() (line 161) calls System.getProperty("os.name") directly, while the existing OperatingSystemProfileActivator reads os.name through context.getSystemProperty(). This means in test or remote-build scenarios where the context simulates a different OS, the executable finder would still use the real OS.
3. java.io.File usage instead of NIO2
The code uses File.separator, File.pathSeparator, and Paths.get() — the codebase has been migrating to NIO2 Path APIs (see PR #11364). Should use Path.of() and java.nio.file equivalents.
4. Dead code
candidateNames() (lines 134–145) is package-private, exposed for tests, but never used by isExecutableInPath(). The candidate logic is duplicated inline in the main method.
5. Test issues
returnsFalseWhenFileIsNotExecutable()conditionally skips on Windows viaSystem.getProperty("os.name")instead of@DisabledOnOs(WINDOWS)getPathValueFallsBackToSystemEnv()testsSystem.getenv("PATH") == System.getenv("PATH")— always passes, doesn't prove correctnesstestExecutableWithAbsolutePath()does.replace('\\', '/')which changes path semantics on Windows
Possible ways forward
-
Restrict to build-time only: ensure
executable()conditions are stripped from consumer POMs duringDefaultConsumerPomBuilderprocessing, similar to how<build>is stripped from profiles. This would make the function usable for local build decisions without polluting published POMs. -
Require explicit opt-in: only allow
executable()in profiles that are marked as non-publishable or build-local, so users must consciously decide whether the condition should survive into the consumer POM. -
Add a model validator warning: emit a warning during
mvn install/mvn deploywhen a consumer POM containsexecutable()conditions, alerting users that the published POM depends on the consumer's environment. -
Document as build-local only: if the function is accepted, document clearly that it should only be used in local profiles and not in POMs intended for publication. Though documentation alone doesn't prevent misuse.
Any of options 1–3 (or a combination) would address the reproducibility concern while preserving the useful functionality.
- Add a warning in DefaultModelValidator if executable() condition is used, noting reproducibility issues
- Add a Javadoc warning to ConditionFunctions.executable()
- Remove System.getenv('PATH') fallback in ExecutableFinder
- Fix OS checking to use ProfileActivationContext instead of System.getProperty
- Convert Paths.get to Path.of
- Remove dead code (candidateNames)
- Update tests: remove fallback test, use @DisabledOnOs(OS.WINDOWS) for OS-specific test
|
Thanks for the detailed review! I completely agree with the reproducibility concerns and the technical points. I've pushed an update that addresses all of them. Reproducibility / Model ValidationTo address the core concern about environment-dependent profile activation in published POMs, I went with Option 3 + Option 4:
Technical Fixes
Everything is pushed to the branch! Let me know if there's anything else needed. |
Implements MNG-8768 (GitHub #10115): adds an executable(name_or_path) function to Maven's condition-based profile activation DSL.
The function evaluates to true if the given executable name can be found in the system PATH, or if an absolute/relative path points directly to an executable file.
Use-case (from the issue): auto-detect the presence of x86_64-linux-musl-gcc when building GraalVM native images with the native-maven-plugin, and conditionally add --static --libc=musl to the build options only when the compiler wrapper is available on the current machine.
Example pom.xml usage:
executable('x86_64-linux-musl-gcc')
Implementation details:
Tests:
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.