#2286: Fix SystemPath.findBinary to search extraPathEntries - #2321
Conversation
krystynaShatkovska
added
CLI
…Entries to work on Linux The test created 'faketool.cmd' which only exists on Windows. On Linux, findBinaryInOrder doesn't try .cmd extensions, so the file was never found, causing the assertion to fail. Fixed by creating a plain 'faketool' file without extension, which works on both Linux (no extension search) and Windows (falls through EXTENSION_PRIORITY to empty extension).
Coverage Report for CI Build 32148279960Coverage remained the same at 72.933%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions5 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
Ali-Shariati-Najafabadi
left a comment
Ali-Shariati-Najafabadi
left a comment
There was a problem hiding this comment.
Logic looks right, extraPathEntries now gets checked before tool2pathMap, matching the order toString() already uses. Tested it against the actual bug path in ProcessContextImpl.run(), ran the full suite locally, all green.
Issue #2286 isn't labeled internal, so it needs a CHANGELOG.adoc entry per the DoD.
The test created 'mytool' as a plain file, but collectToolPath only registers subdirectories (checks Files.isDirectory before adding to tool2pathMap). Changed to create 'mytool' as a subdirectory with the binary file inside, so it's properly registered in tool2pathMap and the test correctly verifies extra PATH entries win over tool2pathMap. Fixes reviewer feedback on PR devonfw#2286.
e5c0098 to
7ee800b
Compare
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@krystynaShatkovska thanks for your PR. Straight forward fix with solid JUnit test - nice job 👍
BTW: The end-user will not have any clue what this story will mean. It would have made mode sense to mark the story as internal. When I do not create the story, I miss such details and do not want to get in the way. I am just trying to raise the awareness in the team so you can understand for yourself what should belong into the CHANGELOG and what not.
For today, I want to get some PRs merged rather than blocking it due to such detailed discussions...
4 participants
This PR fixes #2286
Implemented changes:
withPathEntry()adds directories to the PATH exported to child processes, butSystemPath.findBinary()never searchedextraPathEntries. This caused binaries added this way tofail with
CreateProcess error=2.extraPathEntriesas the first search location infindBinary()— matching the PATH precedence order used bytoString()(extraPathEntries→tool2pathMap→paths)testFindBinaryFindsBinaryInExtraPathEntries()to verify the fixTesting instructions
mvn clean test— all tests (CLI: 848, GUI: 56, URL-Updater: 72) pass with BUILD SUCCESSwithPath(), and verifiesfindBinary()resolves it correctlyChecklist for this PR
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»In Progressand assigned to youpom.xmlfiles