Skip to content

fix: Maven find_executable() walks up from build_root instead of checking CWD#1945

Closed
mashraf-222 wants to merge 1 commit intomainfrom
cf-1079-maven-cwd-fix
Closed

fix: Maven find_executable() walks up from build_root instead of checking CWD#1945
mashraf-222 wants to merge 1 commit intomainfrom
cf-1079-maven-cwd-fix

Conversation

@mashraf-222
Copy link
Copy Markdown
Contributor

Problem

MavenStrategy.find_executable() had a fallback that checked Path("mvnw").exists() against Python's current working directory instead of the build_root parameter. This returned a relative "./mvnw" path that caused FileNotFoundError when the subprocess ran with cwd=<submodule> where no mvnw exists.

Impact: 100% of test executions failed in multi-module Maven projects (Hazelcast, Spring Framework, etc.). In a Hazelcast sweep, 282 functions all crashed at test execution — 4.5 hours wasted.

Root Cause

maven_strategy.py:656-659 — CWD-based check returns a path relative to the wrong directory:

# BUG: checks Python CWD, not build_root
if Path("mvnw").exists():
    return "./mvnw"  # relative to CWD, not build_root

When subprocess later runs with cwd=build_root (the submodule dir), ./mvnw doesn't exist there.

Fix

Replaced the CWD-based fallback with parent-directory traversal — the same pattern GradleStrategy.find_executable() already uses correctly. Walks up from build_root to find mvnw in any ancestor directory, always returning an absolute path.

Validation

Bug replicated before fix:

os.chdir("/home/ubuntu/code/hazelcast")  # repo root — has mvnw
MavenStrategy().find_executable(Path("hazelcast/hazelcast"))
# Returns: "./mvnw" — WRONG (relative to CWD)

Fix confirmed after change:

MavenStrategy().find_executable(Path("hazelcast/hazelcast"))
# Returns: "/home/ubuntu/code/hazelcast/mvnw" — CORRECT (absolute, found in parent)

E2E validation: Fibonacci optimization on java-test-project completed without FileNotFoundError (single-module regression check passed). Hazelcast multi-module scenarios verified via targeted Python tests (3/3 pass).

Test Coverage

  • test_find_wrapper_in_parent_directory — mvnw at repo root, build_root is submodule
  • test_find_wrapper_in_grandparent_directory — mvnw two levels up from build_root
  • test_no_wrapper_returns_system_mvn_or_none — no wrapper anywhere, falls back to system mvn
  • test_cwd_does_not_affect_result — CWD has mvnw but build_root doesn't — no contamination

Closes CF-1079

…king CWD

The CWD-based `Path("mvnw").exists()` fallback returned a relative
"./mvnw" that failed with FileNotFoundError when subprocess ran with
cwd=<submodule>. Now walks up parent directories from build_root,
matching the pattern already used by GradleStrategy.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 1, 2026

Claude finished @mashraf-222's task in 2m 32s —— View job


PR Review Summary

  • Triage PR size → SMALL (12 lines of production code changed)
  • Run lint/typecheck → prek passes, mypy pre-existing issues in test file (unrelated to this PR)
  • Resolve stale threads → none found
  • Review code changes
  • Duplicate detection
  • Handle optimization PRs

Prek Checks

All clean — prek passes with no issues on changed files.

Code Review

The fix is correct and well-implemented.

maven_strategy.py:649-662 — The new implementation exactly mirrors GradleStrategy.find_executable() (gradle_strategy.py:419-435), which is the right pattern for multi-module projects where the wrapper lives at the repo root. Key correctness properties:

  • build_root.resolve() ensures symlinks are resolved before traversal
  • parent == current terminates the walk at the filesystem root (correct on both Unix and Windows)
  • All returned paths are absolute — no more relative "./mvnw" that breaks when subprocess runs with a different cwd
  • Falls back to shutil.which("mvn") — same as before

One minor observation: the old code had a fast-path check for build_root / "mvnw" and build_root / "mvnw.cmd" as the first check (before the CWD fallback). The new loop starts from build_root so this is preserved. No regression.

Teststest_build_tools.py:388-452 — Good coverage of the actual failure scenario. The four new tests cover the key cases:

  • Parent-dir traversal ✓
  • Grandparent-dir traversal ✓
  • No wrapper → system mvn fallback ✓
  • CWD isolation ✓

The test_cwd_does_not_affect_result test uses if result is not None before asserting, which is appropriate since system mvn might be on PATH pointing elsewhere.

No issues found.

Duplicate Detection

The new find_executable in MavenStrategy is now functionally identical to GradleStrategy.find_executable(). Both strategies are intentionally parallel classes, not sharing a base, so this duplication is appropriate (same pattern as the rest of the strategy methods). No unintended duplication introduced.

Optimization PR

PR #1943 (⚡️ Speed up fmt_delta by 11%, base: cf-compare-copy-benchmarks): CI failures include unit-tests and js-ts-class-optimization which are pre-existing on the base branch. Several other checks (async-optimization, bubble-sort, init-optimization, js-cjs) fail on the PR but pass on the base — these may be test flakiness given the trivial nature of the change (%+.0f string formatting is equivalent to {:+.0f} f-string). Leaving open for human review.


@mashraf-222 mashraf-222 closed this Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant