Fix path detection when Build appears in directory names#40
Conversation
The old code used `components(separatedBy: "/Build/")[0]` which splits on
the first occurrence of "/Build/". This fails when usernames or project
paths contain "Build" (e.g., /Users/Build/Projects/App/Build/...).
Now uses `.range(of: "/Build/", options: .backwards)` to find the last
occurrence, matching the behavior of the shell `${x%/Build/*}` pattern.
Also updates the README run script to use the more robust `%/Build/*`
parameter expansion and proper quoting.
Closes #37
There was a problem hiding this comment.
Pull request overview
This PR fixes AckGen’s SPM package cache path detection when "Build" appears earlier in PROJECT_TEMP_DIR (e.g., in a username or parent directory), and updates the README’s Xcode Run Script snippet to compute the AckGen checkout path more robustly.
Changes:
- Update Swift path parsing to locate the last
"/Build/"occurrence (matching${x%/Build/*}behavior). - Update README Run Script to compute the AckGen checkout directory via shell parameter expansion and improve quoting/command substitution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Sources/AckGenCLI/AckGen.swift | Switches package cache base path detection to use a backwards "/Build/" match to avoid incorrect truncation when "Build" appears earlier in the path. |
| README.md | Updates the recommended Xcode Run Script to compute the AckGen checkout path from PROJECT_TEMP_DIR using robust parameter expansion and proper quoting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let packageCachePath: String | ||
| if let range = tempDirPath.range(of: "/Build/", options: .backwards) { | ||
| packageCachePath = String(tempDirPath[..<range.lowerBound]) + "/SourcePackages/checkouts" | ||
| } else { | ||
| packageCachePath = tempDirPath.components(separatedBy: "/Build/")[0] + "/SourcePackages/checkouts" | ||
| } |
There was a problem hiding this comment.
The else branch is redundant and keeps the old components(separatedBy: "/Build/")[0] logic around (which is the behavior this PR is trying to move away from). If "/Build/" isn't present, components(...)[0] is just tempDirPath anyway, so this can be simplified to make the intent clearer (e.g., use tempDirPath directly or compute a basePath once and append /SourcePackages/checkouts).
| let packageCachePath: String | |
| if let range = tempDirPath.range(of: "/Build/", options: .backwards) { | |
| packageCachePath = String(tempDirPath[..<range.lowerBound]) + "/SourcePackages/checkouts" | |
| } else { | |
| packageCachePath = tempDirPath.components(separatedBy: "/Build/")[0] + "/SourcePackages/checkouts" | |
| } | |
| let basePath: String | |
| if let range = tempDirPath.range(of: "/Build/", options: .backwards) { | |
| basePath = String(tempDirPath[..<range.lowerBound]) | |
| } else { | |
| basePath = tempDirPath | |
| } | |
| let packageCachePath = basePath + "/SourcePackages/checkouts" |
| SDKROOT=$(xcrun --sdk macosx --show-sdk-path) | ||
| swift run ackgen |
There was a problem hiding this comment.
In this shell snippet SDKROOT=$(...) only sets a shell variable; it is not exported, so swift run ackgen will not see SDKROOT in its environment. If the intent is to force the macOS SDK for the child process, export it (or set it inline on the swift run invocation); otherwise consider removing the line to avoid implying it has an effect.
| SDKROOT=$(xcrun --sdk macosx --show-sdk-path) | |
| swift run ackgen | |
| SDKROOT=$(xcrun --sdk macosx --show-sdk-path) swift run ackgen |
- Extract path-parsing logic into testable deriveCheckoutsPath() function - Add 6 DeriveCheckoutsPathTests: standard path, archive builds (#6), "Build" in username (#40), multiple Build segments, fallback - Add 5 PlistEncodingTests: both output formats, multiline license preservation, file write round-trip - Fix ackgen.sh: missing $ in SDKROOT command substitution Closes #43 Phase 2
Summary
components(separatedBy: "/Build/")[0]splitting on first occurrence instead of last.range(of: "/Build/", options: .backwards)to match bash${x%/Build/*}behaviorSupersedes #37 (which was based on stale pre-SwiftArgumentParser code and had significant scope creep).
Test plan
/Users/Build/Projects/...)