fix(core): prefer current repo when resolving amber home path#4263
fix(core): prefer current repo when resolving amber home path#4263carloea2 wants to merge 7 commits intoapache:mainfrom
Conversation
|
@chenlica Can you assign a reviewer? Thanks. |
|
@aglinxinyuan @Xiao-zhen-Liu Please review this PR. |
There was a problem hiding this comment.
Pull request overview
Improves Utils.amberHomePath resolution to be more deterministic in environments where multiple sibling repositories may contain an amber/ directory, reducing the chance of selecting the wrong repo when launched from a non-amber working directory.
Changes:
- Adds a two-pass search strategy to prefer a “closest” match before falling back to any match.
- Adds safeguards for filesystem-root cases and normalizes returned paths via
toRealPath(). - Ensances resource safety by closing
Files.walk(...)viaUsing.resource(...).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
Outdated
Show resolved
Hide resolved
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
Outdated
Show resolved
Hide resolved
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
Outdated
Show resolved
Hide resolved
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
Outdated
Show resolved
Hide resolved
|
Just curious, who requested copilot review? |
I requested it. Please address the comments from Copilot before I have a pass. |
|
@aglinxinyuan done. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Preserve the current behavior by preferring an amber directory discovered under the CWD. | ||
| amberCandidates | ||
| .filter(_.startsWith(realCurrentWorkingDirectory)) | ||
| .maxByOption(_.getNameCount) | ||
| .orElse(amberCandidates.headOption) |
There was a problem hiding this comment.
I prefer the current way since the match is deeper, I think both options can be, it depends in the design decision. @aglinxinyuan what is your preference?
There was a problem hiding this comment.
Could you provide an example of a user who would prefer the deeper one instead of the closer one? From my perspective, if I run a command in the current working directory, I would expect the binary in the closest folder to be used rather than the one in the deepest folder.
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
Outdated
Show resolved
Hide resolved
| val siblingRepo = Files.createDirectory(tempDirectory.resolve("sibling-repo")) | ||
| val preferredAmber = Files.createDirectories(preferredRepo.resolve("amber")) | ||
| Files.createDirectories(siblingRepo.resolve("amber")) | ||
|
|
What changes were proposed in this PR?
This PR fixes an issue in
amberHomePathresolution where the previous implementation could select the wrong repo when multiple sibling directories under the same parent satisfiedisAmberHomePath(...).texera/amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
Lines 39 to 59 in ac909a0
Previously, if the current working directory was not itself recognized as the Amber home path, the code would walk the parent directory and return
findAny()from matching paths. In environments with multiple Texera/Amber repos under the same parent, this could resolve to an unrelated sibling repo instead of the repo the process was launched from.This PR changes the logic to:
toRealPath()Files.walk(...)safely usingUsing.resource(...)This makes Amber home path detection more deterministic and prevents accidentally selecting the wrong engine/repo in multi-repo local setups.
Any related issues, documentation, discussions?
Closes #4262
How was this PR tested?
Manually tested with a local directory layout where multiple sibling repos satisfy
isAmberHomePath(...).Example setup:
Was this PR authored or co-authored using generative AI tooling?
No.