java: eliminate git clone into target/ — use monorepo directly#1592
Open
edburns wants to merge 4 commits into
Open
java: eliminate git clone into target/ — use monorepo directly#1592edburns wants to merge 4 commits into
edburns wants to merge 4 commits into
Conversation
The Java SDK now lives inside the monorepo, so cloning the same repo into target/copilot-sdk/ is redundant and causes Windows file-locking issues (AccessDeniedException on .git pack files during mvn clean). Replace the clone-based approach with direct references to the monorepo root (project.basedir/..). This: - Removes the clone-or-update-copilot-sdk antrun execution - Points test harness npm install at ../test/harness - Points CLI npm ci at ../nodejs - Removes the post-clean-sweep workaround (no more .git in target) - Removes the now-obsolete readonly-copilot-sdk-ref-impl-version property (the version is whatever is on the current branch) - Renames copilot.sdk.clone.dir -> copilot.sdk.root
The .lastmerge file tracked which monorepo commit the Java build
should clone from. Now that the Java build uses the monorepo
directly (no clone), .lastmerge serves no purpose.
- Delete java/.lastmerge
- Remove the pathspec exclude (':!java/.lastmerge') from git add
- Remove the post-commit .lastmerge write + amend dance
- Update PR body text to no longer mention .lastmerge
The reference-impl-sync workflow never existed in this monorepo
(it was from the standalone Java repo) so nothing else to remove.
The 'Verify CLI works' step referenced the old target/copilot-sdk/nodejs/... path. Update to use the monorepo's nodejs/ directory directly (../nodejs relative to java/).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Java build/test setup to stop cloning the copilot-sdk repo into java/target/ and instead reference the monorepo directly, primarily to eliminate Windows file-locking failures during mvn clean.
Changes:
- Replaced the clone-based test setup with a
${project.basedir}/..monorepo root reference and updated harness/CLI paths accordingly. - Removed
.lastmergeand related workflow mechanics that existed only to support cloning/pinning a separate repo copy. - Updated Java CI to verify the CLI using the monorepo
nodejs/path.
Show a summary per file
| File | Description |
|---|---|
java/pom.xml |
Removes git-clone-based test setup; points harness + CLI installs to monorepo paths and simplifies clean configuration. |
java/.lastmerge |
Deletes the obsolete commit-pin file used for cloning/resetting a separate repo copy. |
.github/workflows/update-copilot-dependency.yml |
Removes .lastmerge update/amend logic and stages all changes normally. |
.github/workflows/java-sdk-tests.yml |
Updates the CLI verification step to run against ../nodejs/... from the java/ working directory. |
Copilot's findings
Comments suppressed due to low confidence (1)
java/pom.xml:297
- This comment still refers to the CLI being under
target/copilot-sdk/nodejs, but the build now installs/uses the CLI from the monorepo’snodejs/directory viacopilot.sdk.root. Update the comment so it matches the new layout.
<!--
Set COPILOT_CLI_PATH for the forked test JVM so the SDK
tests transparently use the pinned CLI under
target/copilot-sdk/nodejs/. See the copilot.cli.path
property above for the override mechanism.
- Files reviewed: 4/4 changed files
- Comments generated: 1
Comment on lines
204
to
210
| <configuration> | ||
| <skip>${skip.test.harness}</skip> | ||
| <executable>npm</executable> | ||
| <workingDirectory>${copilot.sdk.clone.dir}/test/harness</workingDirectory> | ||
| <workingDirectory>${copilot.sdk.root}/test/harness</workingDirectory> | ||
| <arguments> | ||
| <argument>install</argument> | ||
| </arguments> |
Contributor
Cross-SDK Consistency Review ✅This PR makes no changes to any SDK public API surface. All 4 modified files are Java build/CI infrastructure:
No cross-language consistency concerns. The other SDKs (Node.js, Python, Go, .NET, Rust) have always referenced the shared test harness and CLI directly without a git-clone indirection, so this change brings Java's build process into alignment with how the rest of the monorepo already works.
|
Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
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
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.
Summary
The Java SDK now lives inside the monorepo, so cloning the same repo into \ arget/copilot-sdk/\ during \generate-test-resources\ is redundant and causes Windows file-locking issues (\AccessDeniedException\ on .git\ pack files during \mvn clean).
Changes
POM (\java/pom.xml)
pm install\ at ../test/harness\ instead of \ arget/copilot-sdk/test/harness\
pm ci\ at ../nodejs\ instead of \ arget/copilot-sdk/nodejs\
Workflow (.github/workflows/update-copilot-dependency.yml)
Workflow (.github/workflows/java-sdk-tests.yml)
Deleted
Testing
All 1696 tests pass locally (0 failures, 5 skipped). The only build failure is a pre-existing \�erify-java25-overlay\ multi-release JAR packaging issue unrelated to this change.