-
Notifications
You must be signed in to change notification settings - Fork 76
Try loading Swift libraries from JAR resources #511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Modified library loading mechanism to attempt loading from JAR resources before falling back to system library paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
This PR does work for me for when I launch my app from a jar. The java project building swift wrappers must add the build binaries as resources (see https://github.com/TheTripleV/photonvision/blob/mac/photon-apple/build.gradle#L73). Then, the generated wrappers will load binaries from jar if they are not available on the java library path. --
For reference, the directory structure of my built jar: LICENSE
META-INF
ResourceInformation.json
boarddefs
calibration
com
controller.proto
edu
geometry2d.proto
geometry3d.proto
io
jakarta
javax
jetty-dir.css
kinematics.proto
kotlin
lib
libPhotonAppleLibrary.dylib
libSwiftJava.dylib
libSwiftRuntimeFunctions.dylib
models
org
osx
pabeles
photon.proto
plant.proto
soc_mapping.properties
spline.proto
sqlite-jdbc.properties
system.proto
testimages
tinylog.properties
trajectory.proto
us
web
wpimath.proto |
| try { | ||
| SwiftLibraries.loadResourceLibrary(SwiftLibraries.LIB_NAME_SWIFT_CORE); | ||
| } catch (Throwable e2) { | ||
| if (PlatformUtils.isMacOS()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also handle linux here; it would be under /usr/lib/swift/linux/libswiftCore.so
| } catch (Throwable e2) { | ||
| if (PlatformUtils.isMacOS()) { | ||
| try { | ||
| System.load("/usr/lib/swift/libswiftCore.dylib"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a function like SwiftLibraries.libSwiftCorePath() and inside it make the is.... platform checks, and return the appropriate path for the platform.
The func there should then find the right prefix (/usr/lib/swift/linux/, /usr/lib/swift) based on platform, and do the same to the suffix (so, dylib). We don't want those paths just hardcoded in many places like this
| System.loadLibrary(SwiftLibraries.LIB_NAME_SWIFT_JAVA); | ||
| } catch (Throwable e) { | ||
| SwiftLibraries.loadResourceLibrary(SwiftLibraries.LIB_NAME_SWIFT_JAVA); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty repetitive, please make a function inside SwiftLibraries that does the "attempt path, then resource" along with nice error reporting
| throw new RuntimeException("Expected library '" + libname + "' ('" + resourceName + "') was not found as resource!"); | ||
| } | ||
|
|
||
| // TODO: we could do an in memory file system here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO is still valid, don't remove it
| // Cache of already-loaded libraries to prevent duplicate extraction | ||
| private static final java.util.Map<String, File> loadedLibraries = new java.util.HashMap<>(); | ||
|
|
||
| public static synchronized void loadResourceLibrary(String libname) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use synchronized this'll bottleneck loading libraries while there's no reason to do so at all -- we can use the loadded libraries map but please back it with a ConcurrentHashMap for safety and remove the synchronized.
| System.load(tempFile.getAbsolutePath()); | ||
|
|
||
| // Cache the loaded library | ||
| loadedLibraries.put(libname, tempFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make use of concurrent hashmap's computeIfAbsent here please
|
|
||
| @SuppressWarnings("unused") | ||
| private static final boolean INITIALIZED_LIBS = loadLibraries(false); | ||
| private static final boolean INITIALIZED_LIBS = AUTO_LOAD_LIBS ? loadLibraries(false) : true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems right, thanks
ktoso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Needs some cleanup, thank you for the work though. Marking as needs changes until comments addressed
closes #510
For swiftCore:
For the rest: