Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Add LMDB's POSIX semaphore feature #757

Closed
wants to merge 5 commits into from
Closed

Add LMDB's POSIX semaphore feature #757

wants to merge 5 commits into from

Conversation

GregoryConrad
Copy link
Contributor

Hi! Please take a look at this PR first. It will need to be merged before this PR can be merged.

In summary, this PR adds support for running milli on iOS and macOS when using App Sandbox, which is required when submitting to the App Store (and required for all iOS apps; macOS apps can get away without needing the App Sandbox but it's recommended).

Under the hood, this is done by adding a cargo feature to use LMDB's POSIX semaphores instead of SysV semaphores (which is LMDB's default). POSIX semaphores comply with Apple's App Sandbox and actually might bring slight performance improvements on *nix platforms.

@GregoryConrad
Copy link
Contributor Author

GregoryConrad commented Dec 24, 2022

It's also worth mentioning I didn't really want to add this feature to milli directly, since it is not exactly a milli feature. However; based on what I read, cargo does not support adding/setting features of transitive dependencies (in this case heed & lmdb-rs), which means it should be promoted to a milli feature (unless someone else has a better idea).

I.e., I can't depend on milli but also specify that I want to use heed with posix-sem; I have to tell milli to use heed with posix-sem.

@GregoryConrad GregoryConrad changed the title fix: add POSIX semaphore feature Add LMDB's POSIX semaphore feature Jan 5, 2023
@GregoryConrad
Copy link
Contributor Author

This PR is ready to go as soon as a new version of heed is released.

@GregoryConrad GregoryConrad marked this pull request as ready for review January 5, 2023 22:37
@GregoryConrad
Copy link
Contributor Author

@Kerollmops I noticed a-new-era over in heed was merged, which has implications for this PR since it was set to use v0.12 still.

I removed the modification on the heed version in this PR since it looked like it might be wrong now; please let me know if anything else needs to be done to this PR.

@Kerollmops
Copy link
Member

Hey @GregoryConrad,

I noticed a-new-era over in heed was merged, which has implications for this PR since it was set to use v0.12 still.

Don't worry, I merged your commit on main too! Just so you know, we will not use v0.20.0 of heed on milli for now.

I removed the modification on the heed version in this PR since it looked like it might be wrong now; please let me know if anything else needs to be done to this PR.

I just did a release of v0.12.5. Sorry for the time I took. I was indeed working on the new v0.20.0 release.

@GregoryConrad
Copy link
Contributor Author

GregoryConrad commented Jan 16, 2023

Just so you know, we will not use v0.20.0 of heed on milli for now.

Ok; I will revert my commit 4c4f9a6 to make this PR point to v0.12.5!

@curquiza
Copy link
Member

Hello @GregoryConrad thanks a lot for your work and your involvement.

For your information, we are closing this repo soon. We don't accept any contribution. However, the code base of this repo is moved to the meilisearch repository, under the milli workspace.
Could you move your PR to Meilisearch please? 😇

Thanks again for your involvement in the project, it's nice to have you around! 👋

@curquiza curquiza closed this Jan 19, 2023
@GregoryConrad
Copy link
Contributor Author

@curquiza Sure, I'll move the changes over there right now!

bors bot added a commit to meilisearch/meilisearch that referenced this pull request Feb 6, 2023
3407: Add Cargo feature for LMDB's POSIX semaphores r=dureuill a=GregoryConrad

See meilisearch/milli#757

Co-authored-by: Gregory Conrad <[email protected]>
bors bot added a commit to meilisearch/meilisearch that referenced this pull request Feb 6, 2023
3407: Add Cargo feature for LMDB's POSIX semaphores r=dureuill a=GregoryConrad

See meilisearch/milli#757

Co-authored-by: Gregory Conrad <[email protected]>
bors bot added a commit to meilisearch/meilisearch that referenced this pull request Feb 7, 2023
3407: Add Cargo feature for LMDB's POSIX semaphores r=dureuill a=GregoryConrad

See meilisearch/milli#757

Co-authored-by: Gregory Conrad <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants