Skip to content

fix(core): prefer current repo when resolving amber home path #4263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
carloea2 wants to merge 3 commits into
base: main
Choose a base branch
from
+38 −12
Open

fix(core): prefer current repo when resolving amber home path #4263

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df17bea
v1
carloea2 Mar 7, 2026
831029b
Merge branch 'main' into fix/amber_home_path
carloea2 Mar 7, 2026
6ed1155
Merge branch 'main' into fix/amber_home_path
aglinxinyuan Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 38 additions & 12 deletions amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ package org.apache.texera.amber.engine.common
import com.typesafe.scalalogging.LazyLogging
import org.apache.texera.amber.engine.architecture.rpc.controlreturns.WorkflowAggregatedState

import java.nio.file.{Files, Path, Paths}
import java.util.concurrent.locks.Lock
import scala.annotation.tailrec

Expand All @@ -36,25 +35,52 @@ object Utils extends LazyLogging {
*
* @return the real absolute path to amber home directory
*/

import java.nio.file.{Files, Path, Paths}
import scala.jdk.CollectionConverters._
import scala.util.Using
Comment on lines +39 to +41
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new imports are placed inside the Utils object, while other files in this package keep imports at the top of the file. Consider moving them to the top-level import section for consistency and to make dependencies easier to spot.

Copilot uses AI. Check for mistakes.

lazy val amberHomePath: Path = {
val currentWorkingDirectory = Paths.get(".").toRealPath()
// check if the current directory is the amber home path

if (isAmberHomePath(currentWorkingDirectory)) {
currentWorkingDirectory
} else {
// from current path's parent directory, search its children to find amber home path
// current max depth is set to 2 (current path's siblings and direct children)
val searchChildren = Files
.walk(currentWorkingDirectory.getParent, 2)
.filter((path: Path) => isAmberHomePath(path))
.findAny
if (searchChildren.isPresent) {
searchChildren.get
} else {
val parent = Option(currentWorkingDirectory.getParent).getOrElse {
throw new RuntimeException(
"Finding texera home path failed. Current working directory is " + currentWorkingDirectory
s"Cannot search for texera home from filesystem root: $currentWorkingDirectory"
)
Comment on lines +49 to 52
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new exception message refers to "texera home" even though this method is computing amberHomePath and the scaladoc says "amber home directory". Aligning the wording (e.g., "Amber home" or "Texera/Amber home") would make this error easier to understand when it appears in logs.

Copilot uses AI. Check for mistakes.
}

// Pass 1: prefer the closest prefix (deepest ancestor) of currentWorkingDirectory
val closestPrefix: Option[Path] =
Using.resource(Files.walk(parent, 2)) { stream =>
stream
.iterator()
.asScala
.filter(path => isAmberHomePath(path))
.map(_.toRealPath()) // normalize after filtering
.filter(path => path.startsWith(currentWorkingDirectory))
.maxByOption(_.getNameCount) // deepest prefix = closest ancestor
}
Comment on lines +55 to +65
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment/variable name implies selecting the closest prefix/ancestor of currentWorkingDirectory, but the predicate uses path.startsWith(currentWorkingDirectory) which instead selects matches under the working directory. If the intent is deepest ancestor (e.g., when CWD is inside an amber/... subtree), the startsWith direction should be reversed (or handle both ancestor/descendant cases explicitly) to avoid selecting the wrong candidate in edge cases with multiple matches.

Copilot uses AI. Check for mistakes.

closestPrefix.getOrElse {
// Pass 2: fallback to any valid match
val anyMatch =
Using.resource(Files.walk(parent, 2)) { stream =>
stream
.filter((path: Path) => isAmberHomePath(path))
.findAny()
}
Comment on lines +56 to +74
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files.walk(parent, 2) is executed twice (pass 1 and pass 2). Since directory walking can be relatively expensive (and can observe different FS state between passes), consider walking once, collecting the matching candidates, and then applying the preferred-selection logic + fallback in-memory.

Copilot uses AI. Check for mistakes.

if (anyMatch.isPresent) {
anyMatch.get().toRealPath()
} else {
throw new RuntimeException(
s"Finding texera home path failed. Current working directory is $currentWorkingDirectory"
)
}
}
}
Comment on lines 43 to 84
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change alters path-resolution behavior in a way that’s easy to regress (especially with multiple sibling repos). Since the amber module already has ScalaTest coverage, adding a focused unit test that creates a temp directory tree and sets user.dir to validate the selection logic would help prevent future nondeterministic failures.

Copilot uses AI. Check for mistakes.
}
val AMBER_HOME_FOLDER_NAME = "amber";
Expand Down
Loading