Skip to content
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

Using library version to unpack JVM libs #1637

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Using library version to unpack JVM libs #1637

merged 3 commits into from
Jan 31, 2024

Conversation

nhachicha
Copy link
Collaborator

Using library version to build a path where to unpack native libraries for JVM targets

Fixes #1617

@nhachicha nhachicha self-assigned this Jan 19, 2024
@nhachicha nhachicha marked this pull request as ready for review January 19, 2024 14:16
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I do have some reservations about the CHANGELOG though and I think you are duplicating creating a version string.

CHANGELOG.md Outdated
@@ -28,6 +28,7 @@
### Internal
* Update to Ktor 2.3.4.
* Updated to CMake 3.27.7
* The Unpacking of JVM native library will use the current library version instead of a calculated hash for the path.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved to the public part of the changelog? At least it seems like it might have an impact on people that might depended on the old behavior and now need to manually put files in some other location (if they did that before)

defaultConfigs {
buildConfigField(Type.STRING, "version", Realm.version)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

@@ -792,3 +749,12 @@ tasks.named("clean") {
delete(project.file(".cxx"))
}
}

// Generate an object holding the current release version, to be used by the JVM SoLoader path construction
buildkonfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already being generated by cinterop here: https://github.com/realm/realm-kotlin/blob/main/packages/cinterop/build.gradle.kts#L796, so I do not think you need to add this plugin for it?

libraryHash + File.separator +
platform.defaultSystemLocation +
File.separator +
LibraryConfig.version +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about this already being created by our cinterop library

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 💯

@nhachicha nhachicha merged commit 3d7df67 into main Jan 31, 2024
53 checks passed
@nhachicha nhachicha deleted the nh/jvm_check_flag branch January 31, 2024 16:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate and support new behavior by Conveyor for packing desktop apps.
2 participants