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

fix: use dylib extension instead of jnilib for MacOS library names #939

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

kkriske
Copy link
Contributor

@kkriske kkriske commented Jul 20, 2023

copy from #922 (comment):
The libraries for MacOS are packaged as a .jnilib file. This seems legacy from pre java 7 (https://bugs.openjdk.org/browse/JDK-7134701). The default naming now seems to be .dylib.

@kkriske kkriske mentioned this pull request Jul 20, 2023
@gotson gotson self-assigned this Jul 21, 2023
Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably don't need that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change does nothing indeed, but windows is unable to create the exact same binary twice due to embedded metadata like creation timestamps. I can revert the changes to the dlls if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can revert the changes to the dlls if you want.

that's what i was implying. This PR should not modify those files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably don't need that one

Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably don't need that one

Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably don't need that one

Copy link
Collaborator

Choose a reason for hiding this comment

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

was that regenerated or just renamed ? file size has changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed them initially, and then ran the CI to validate if they are still created correctly.
I don't know why the file size is different.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we keep the original file and just rename them in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the CI run, that should do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

was that regenerated or just renamed ? file size has changed

@gotson gotson merged commit 78defd0 into xerial:master Jul 24, 2023
44 checks passed
@gotson gotson changed the title use *.dylib extension instead of *.jnilib for MacOS library names fix: use dylib extension instead of jnilib for MacOS library names Jul 24, 2023
@kkriske kkriske deleted the change-jnilib-to-dylib branch July 24, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants