Fix platform-agnostic native artifact check in build backend#326
Open
holodorum wants to merge 2 commits intokson-org:mainfrom
Open
Fix platform-agnostic native artifact check in build backend#326holodorum wants to merge 2 commits intokson-org:mainfrom
holodorum wants to merge 2 commits intokson-org:mainfrom
Conversation
The build backend checked for native artifacts using `any()` across all platforms (win32, darwin, linux), so a stale artifact from a different OS (e.g. `libkson.dylib` left over on a Linux builder) would skip the Gradle build, leaving the actually-needed native library uncompiled. Additionally, the artifact check included `kson_api.h` which is not produced by the build—the Gradle `CopyNativeArtifactsTask` produces `jni_simplified.h` and the runtime loads `jni_simplified.h`. This would have caused fresh wheel builds to always trigger a rebuild attempt. Finally, when both artifacts and `kson-sdist` were absent, the function silently returned, producing broken wheels with no error. Fixes: - Check only the native library for *this* platform, plus `jni_simplified.h` (the header actually produced and loaded at runtime) - Add a post-condition that errors clearly when required artifacts are missing, guiding users to install from a pre-built wheel or ensure a JDK is available - Use `subprocess.run(cwd=...)` instead of `os.chdir` for thread safety - Log both stdout and stderr on Gradle build failure for easier debugging - Exclude platform-specific native binaries from `prepareSdistBuildEnvironment` in build.gradle.kts so sdists don't bundle stale cross-platform artifacts Fixes kson-org#307
Collaborator
|
Sorry, misunderstood what was going on. Now I see this is a Python only thing. Glad to see improvements, nevertheless :) |
PixiExecTask only declares inputs (command strings, env vars) but no outputs, so Gradle always considered this task out-of-date and re-ran the full native-image build every time. Add explicit input/output declarations so Gradle can properly cache the task: the project JAR, runtime classpath, and JNI config as inputs, and the native binary as the output. The output is the specific binary file rather than the whole directory, since krossover's generateJniBindingsJvm also writes into that directory.
holodorum
force-pushed
the
build-issue
branch
from
a7a9bbf to
b50ac89
Compare
Collaborator
Author
|
That's another problem indeed, but adding an output directory in b50ac89 for the build artifacts stops the re-triggering |
holodorum
changed the title
holodorum
changed the title
holodorum
force-pushed
the
build-issue
branch
from
70df14c to
b50ac89
Compare
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
2 participants
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 build backend's native artifact check had three interrelated bugs that could produce broken wheels:
Stale cross-platform artifacts fooled the check. The old code used
any()across all platform libraries (win32, darwin, linux), so alibkson.dylibleft over from a macOS build on a Linux builder would satisfy the check, skipping the Gradle build entirely and leavinglibkson.souncompiled.The header file check referenced
kson_api.h, which isn't produced by the build.CopyNativeArtifactsTaskproducesjni_simplified.h, and the runtime loadsjni_simplified.h. This meant fresh builds (without a stalekson_api.hon disk) would always think artifacts were missing and attempt a rebuild—or worse, silently succeed without the header.Missing artifacts with no
kson-sdistproduced broken wheels silently. When both the artifacts and thekson-sdistbuild directory were absent, the function just returned without error. The resulting wheel would install but fail at import time.The fix checks only the native library for the current platform plus
jni_simplified.h, adds a post-condition that errors clearly when required artifacts are missing, and excludes platform-specific binaries fromprepareSdistBuildEnvironmentso sdists don't carry stale cross-platform artifacts. A few smaller improvements tag along:subprocess.run(cwd=...)replacesos.chdirfor thread safety, and Gradle build failures now log both stdout and stderr.Fixes #307