Fix executable extensions for NDK access from Ferric #135
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Ferric CLI’s cargo configuration to correctly append Windows file extensions when invoking NDK toolchain binaries on Windows.
- Introduce
cmdMaybeandexeMaybeto conditionally add.cmdor.exeon Windows - Append
cmdMaybeto all clang/clang++ invocations andexeMaybetollvm-ar/llvm-ranlib - (Unrelated) PATH concatenation remains unchanged
Comments suppressed due to low confidence (2)
packages/ferric/src/cargo.ts:171
- There are no existing tests covering the Windows-specific behavior of
cmdMaybe/exeMaybe. Consider adding unit tests to verify correct suffixes on Windows targets.
const exeMaybe = process.platform === "win32" ? ".exe" : "";
packages/ferric/src/cargo.ts:210
- On Windows, PATH segments should be joined with
;instead of:. Consider usingpath.delimiterto handle this cross-platform.
PATH: `${toolchainBinPath}:${process.env.PATH}`,
| `aarch64-linux-android${androidApiLevel}-clang${cmdMaybe}` | ||
| ), | ||
| CARGO_TARGET_ARMV7_LINUX_ANDROIDEABI_LINKER: joinPathAndAssertExistence( | ||
| toolchainBinPath, | ||
| `armv7a-linux-androideabi${androidApiLevel}-clang` | ||
| `armv7a-linux-androideabi${androidApiLevel}-clang${cmdMaybe}` | ||
| ), | ||
| CARGO_TARGET_X86_64_LINUX_ANDROID_LINKER: joinPathAndAssertExistence( | ||
| toolchainBinPath, | ||
| `x86_64-linux-android${androidApiLevel}-clang` | ||
| `x86_64-linux-android${androidApiLevel}-clang${cmdMaybe}` | ||
| ), | ||
| CARGO_TARGET_I686_LINUX_ANDROID_LINKER: joinPathAndAssertExistence( | ||
| toolchainBinPath, | ||
| `i686-linux-android${androidApiLevel}-clang` | ||
| `i686-linux-android${androidApiLevel}-clang${cmdMaybe}` | ||
| ), | ||
| TARGET_CC: joinPathAndAssertExistence( | ||
| toolchainBinPath, | ||
| `${targetArch}-linux-${targetPlatform}-clang` | ||
| `${targetArch}-linux-${targetPlatform}-clang${cmdMaybe}` | ||
| ), | ||
| TARGET_CXX: joinPathAndAssertExistence( | ||
| toolchainBinPath, | ||
| `${targetArch}-linux-${targetPlatform}-clang++` | ||
| `${targetArch}-linux-${targetPlatform}-clang++${cmdMaybe}` |
There was a problem hiding this comment.
Clang in the Android NDK on Windows is a native .exe, not a batch .cmd. You should use exeMaybe instead of cmdMaybe when appending the extension for clang/clang++.
There was a problem hiding this comment.
This is not correct.
| const cmdMaybe = process.platform === "win32" ? ".cmd" : ""; | ||
| const exeMaybe = process.platform === "win32" ? ".exe" : ""; |
There was a problem hiding this comment.
[nitpick] The platform-suffix logic is repeated in two separate variables—consider extracting this into a small helper function to reduce duplication and clarify intent.
| const cmdMaybe = process.platform === "win32" ? ".cmd" : ""; | |
| const exeMaybe = process.platform === "win32" ? ".exe" : ""; | |
| const cmdMaybe = getPlatformSuffix("cmd"); | |
| const exeMaybe = getPlatformSuffix("exe"); |
There was a problem hiding this comment.
nit: Maybe const isWindows = process.platform == "win32";?
| const cmdMaybe = process.platform === "win32" ? ".cmd" : ""; | ||
| const exeMaybe = process.platform === "win32" ? ".exe" : ""; |
There was a problem hiding this comment.
nit: Maybe const isWindows = process.platform == "win32";?
Merging this PR will:
.exeor.cmdon Windows.