#2286: Fix SystemPath.findBinary to search extraPathEntries - #2321
Open
krystynaShatkovska wants to merge 7 commits into
Open
#2286: Fix SystemPath.findBinary to search extraPathEntries#2321krystynaShatkovska wants to merge 7 commits into
krystynaShatkovska wants to merge 7 commits into
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).
Collaborator
Coverage Report for CI Build 31786549548Warning No base build found for commit Coverage: 72.93%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Ali-Shariati-Najafabadi
left a comment
Ali-Shariati-Najafabadi
left a comment
Contributor
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.
krystynaShatkovska
force-pushed
the
feature/issue-2286-systempath-findbinary-extra
branch
from
August 14, 2026 09:04
e5c0098 to
7ee800b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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