Hiteshsai007 commented on PR #12332:
URL: https://github.com/apache/maven/pull/12332#issuecomment-4770307935
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 Validation
To address the core concern about environment-dependent profile activation
in published POMs, I went with **Option 3 + Option 4**:
1. **Model Validator Warning**: Added a check in `DefaultModelValidator`
(`validate30RawProfileActivation`). If a profile's `<condition>` contains
`executable(`, Maven will now emit a `WARNING` during model validation (e.g.,
during `mvn install`/`deploy`). The warning explicitly states: *"Profile
activation relies on the 'executable' function, which makes the profile
activation environment-dependent. This means the published POM will not be
reproducible. Consider using this function only in local build profiles or
stripping it before publication."*
2. **Documentation**: Added a strong Javadoc warning to
`ConditionFunctions.executable()` explaining the reproducibility risk and
advising against using it in publishable POMs.
### Technical Fixes
1. **Removed `System.getenv("PATH")` fallback**:
`ExecutableFinder.getPathValue()` now exclusively relies on
`context.getSystemProperty("env.PATH")`.
2. **Fixed OS Detection**: `ExecutableFinder` now reads `os.name` via the
`ProfileActivationContext` instead of directly accessing `System.getProperty`.
3. **NIO2 Migration**: Replaced all `Paths.get()` usages with `Path.of()`
and removed unnecessary OS path separation manipulation.
4. **Cleaned up Dead Code**: Removed the unused package-private
`candidateNames()` method and its corresponding tests since the logic was
duplicated inline anyway.
5. **Fixed Test Issues**:
- Replaced conditional Windows logic in
`returnsFalseWhenFileIsNotExecutable` with JUnit 5's
`@DisabledOnOs(OS.WINDOWS)`.
- Removed the trivial/useless `getPathValueFallsBackToSystemEnv` test.
- Removed the `.replace('\\', '/')` logic in the absolute path test.
Everything is pushed to the branch! Let me know if there's anything else
needed.
--
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]
