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

Cannot build MeiliSearch v0.19.0 as NixOS package #1273

Closed
Michcioperz opened this issue Mar 1, 2021 · 19 comments
Closed

Cannot build MeiliSearch v0.19.0 as NixOS package #1273

Michcioperz opened this issue Mar 1, 2021 · 19 comments
Labels
bug Something isn't working as expected

Comments

@Michcioperz
Copy link

Describe the bug
I tried to use MeiliSearch on my NixOS server today, but noticed the official repos only have v0.9, so I tried to update it.
I took the build script from the official repository and upped the version and fixed the hash of the source code.
The build process stops with an error as seen below.

To Reproduce
Steps to reproduce the behavior:

  1. Run NixOS 20.09
  2. Download this Nix expression: https://gist.github.com/michcioperz/4ecdeaf7921d63784eedf7197eafcb71
  3. Run nix-build meilisearch.nix (the filename you saved to)
  4. An error like this should show up during the vendoring phase:
error: failed to sync

Caused by:
  found duplicate version of package `pest v2.1.3` vendored from two sources:

        source 1: https://github.com/pest-parser/pest.git?rev=51fd1d49f1041f7839975664ef71fe15c7dcaf67#51fd1d49
        source 2: registry `https://github.com/rust-lang/crates.io-index`
Traceback (most recent call last):
  File "/nix/store/m6x1vskdlgsray23j2m2k5j7xzgcjhry-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 42, in <module>
    main()
  File "/nix/store/m6x1vskdlgsray23j2m2k5j7xzgcjhry-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 17, in main
    assert list(data.keys()) == ["source"]
AssertionError
builder for '/nix/store/vm7f4lngx8ar6qbh0n1snrwrscldwab8-meilisearch-0.19.0-vendor.tar.gz.drv' failed with exit code 1
cannot build derivation '/nix/store/cjxkk5frkqdikx6rzvyjlbviz2mlqd99-meilisearch-0.19.0.drv': 1 dependencies couldn't be built
error: build of '/nix/store/cjxkk5frkqdikx6rzvyjlbviz2mlqd99-meilisearch-0.19.0.drv' failed

Expected behavior
The vendoring process should finish and Nix should complain that the hash of the vendor directory is different from what I told it to expect.

Desktop:

  • OS: NixOS 20.09

Additional context
NixOS is really into reproducible builds and pinning hashes of all things that are downloaded, so the first part of the process of building a package of something in Rust is vendoring the dependencies and checking that the hash of vendor directory matches what was given in the build script.
Vendoring doesn't complete successfully, because there are two colliding definitions of pest 2.1.3 package: a crates.io version depended on by pest_derive and a few other pest_* crates, and a git version depended on by meilisearch directly.
If I understand correctly the problem would go away if pest had a new release on crates.io so you wouldn't have to use a git version, but its maintainer is busy as seen in pest-parser/pest#491 and pest-parser/pest#454

@curquiza
Copy link
Member

curquiza commented Mar 2, 2021

Hello @Michcioperz!

Thanks for this report!
We indeed are waiting for the new release of pest! We answered in the pest issue (pest-parser/pest#491 (comment))! 🙂

@happysalada
Copy link
Contributor

I see the label that the issue will be fixed with the next release. I'm keen to test this when the new release comes.

@curquiza
Copy link
Member

curquiza commented Apr 7, 2021

Hello @happysalada!
I miss-clicked one week ago, but this issue will not be probably fixed in the next release, sorry. Indeed, this issue depends on the pest package release.

@happysalada
Copy link
Contributor

Yes that's what I understood. When you think it can be tested, just let me know and I'll give it a try.

@curquiza
Copy link
Member

curquiza commented Apr 8, 2021

Thanks @happysalada! :)

@Michcioperz
Copy link
Author

The invitation to the Pest Discord in the issue about the Pest working group has expired, so I can't see if there have been any updates on the Pest situation.

But I just realized this could be worked around a bit differently Michcioperz@68e4a52 Instead of only changing the version of pest used by meilisearch-core, I'm overriding the version of pest used in the whole dependency tree. This includes the version used by pest_derive.

I tried running a build now locally to see if it solves my issue, but I ran out of disk space at the very end, so I'm gonna try again on a different machine in the morning (it's pretty late now and I don't wanna wake my neighbours up with a rack server, haha). At the very least, vendoring runs successfully with this patch, and the build process continues.

@Michcioperz
Copy link
Author

Adding this patch to the build expression from Nixpkgs and bumping the version to v0.20.0 gives me a working build of meilisearch at the cost of having no less than 4 GBs (probably more) of intermediate build files in /tmp (so technically earlier I ran out of tmpfs space, not actual disk).

@Br1ght0ne It appears that you are the maintainer of the Nix package of Meilisearch. Do you think it would be a good idea to put my solution there? (If yes and if you want to take it, feel free to.)

@happysalada
Copy link
Contributor

happysalada commented Jun 23, 2021

I was checking the commit referenced on the pest dependency, it's from september 2020.
https://github.com/pest-parser/pest/commits/51fd1d49f1041f7839975664ef71fe15c7dcaf67

Perhaps the dependency can just be updated to the latest version (2.1.3 which is from March 2021).

Do you think the update could be made as a PR to this repo?
Rather than having a patch in nixpkgs that someone will have to remember what it's for and when to remove it, having the patch upstream is the best way.
Even if merging the PR takes time, we can always update the version in nixpkgs to point to a particular commit of meilisearch.

my understanding here is that it would be a PR to update the version of one dependency. Let me know if I misinterpreted.

@MarinPostma
Copy link
Contributor

@Michcioperz @happysalada, this is still on my TODO. I have joined the pest group, but hadn't much time to work on it since then. We need to make a new pest release, but there are still some issue we want to merge before we are able to do it. Sorry for it taking so long :(

@Michcioperz
Copy link
Author

@happysalada I don't know if it is a good practice to use the patching functionality in Cargo to replace packages like this, but perhaps this isn't something that can be quickly mitigated with good practices either. So maybe it would actually be viable to merge my patch to Meili for the time being?

I understand your concern about adding the patch to Nixpkgs.

Regarding 2.1.3, it's from March 2020 unfortunately, not 2021, which is why it's not good enough

@happysalada
Copy link
Contributor

happysalada commented Jun 23, 2021

@Michcioperz thanks for clarifying the date, I completely missed that.
I was checking latest version on nixpkgs and it's 0.9.0, perhaps we can bump to an intermediate version between 0.9.0 and 0.20.0 ? (that doesn't have the pest problem)

@MarinPostma no worries at all. Thank you in the first place for making an amazing software! ;-)

@Michcioperz
Copy link
Author

@happysalada According to my understanding, MS has used a modified version of Pest starting in 0.10.0, as seen in commit dcf1096 and by my intuition, that version of Pest also causes the same problem.

If you need a more up-to-date version of MS on Nix, my recommendation for now would be to export my commit to a patch file, copy the upstream build expression to an overlay, add something like cargoPatches = [ ./0001-Patch-pest-better.patch ]; to include the patch from a local file, and then build the package.

@Michcioperz
Copy link
Author

Or an alternative would be to upstream my patch to MeiliSearch, to change the method of inserting the custom version of Pest. I think it should be functionally equivalent, and might even make the build process very slightly faster (because now the original pest 2.1.3 is also built as a dependency of pest_derive, and with my patch pest 2.1.3 is completely replaced by the custom pest)

@happysalada
Copy link
Contributor

happysalada commented Jun 23, 2021

I was checking pest upstream as well and they were organising for a new release.
I'm a little busy at the moment, but will keep the patch option in mind.
Thank you for your help on this!

@curquiza curquiza added the bug Something isn't working as expected label Aug 9, 2021
@happysalada
Copy link
Contributor

I've just made a PR to try to integrate the fix you proposed #1659

bors bot added a commit that referenced this issue Sep 8, 2021
1659: deps: unify pest dependency r=MarinPostma a=happysalada

meilisearch dependends on two different versions of pest.
This can be problematic for some build systems (e.g. NixOS).
Since the repo hasn't received an update in a while, in the meantime, use the later version of the two pest dependencies.

Context: this has been discussed previously #1273
meilisearch has been selected by ngi to be packaged for nixos. A patch can be applied to make the changes proposed in this PR. This PR intends to see how the maintainers of meilisearch would feel about the patch.

What was done.
- Add an override for the pest dependency in Cargo.toml.
- recreate the Cargo.lock with `cargo update`. This has had the side effect of updating some dependencies.

I ran the tests on darwin. My machine is quite old so I had 8 failures due to a timeout. None of the failures look like they are due to the new dependencies.

Checking the pest repo, it seems there are some recent commits, however no sure date of when there could be a new release.

If this gets accepted, there is no need to do a new release, nixos can just target the new commit.

If you feel it's too much pain for not enough gain, no worries at all!

Co-authored-by: happysalada <raphael@megzari.com>
bors bot added a commit that referenced this issue Sep 14, 2021
1659: deps: unify pest dependency r=curquiza a=happysalada

meilisearch dependends on two different versions of pest.
This can be problematic for some build systems (e.g. NixOS).
Since the repo hasn't received an update in a while, in the meantime, use the later version of the two pest dependencies.

Context: this has been discussed previously #1273
meilisearch has been selected by ngi to be packaged for nixos. A patch can be applied to make the changes proposed in this PR. This PR intends to see how the maintainers of meilisearch would feel about the patch.

What was done.
- Add an override for the pest dependency in Cargo.toml.
- recreate the Cargo.lock with `cargo update`. This has had the side effect of updating some dependencies.

I ran the tests on darwin. My machine is quite old so I had 8 failures due to a timeout. None of the failures look like they are due to the new dependencies.

Checking the pest repo, it seems there are some recent commits, however no sure date of when there could be a new release.

If this gets accepted, there is no need to do a new release, nixos can just target the new commit.

If you feel it's too much pain for not enough gain, no worries at all!

Co-authored-by: happysalada <raphael@megzari.com>
bors bot added a commit that referenced this issue Sep 15, 2021
1659: deps: unify pest dependency r=MarinPostma a=happysalada

meilisearch dependends on two different versions of pest.
This can be problematic for some build systems (e.g. NixOS).
Since the repo hasn't received an update in a while, in the meantime, use the later version of the two pest dependencies.

Context: this has been discussed previously #1273
meilisearch has been selected by ngi to be packaged for nixos. A patch can be applied to make the changes proposed in this PR. This PR intends to see how the maintainers of meilisearch would feel about the patch.

What was done.
- Add an override for the pest dependency in Cargo.toml.
- recreate the Cargo.lock with `cargo update`. This has had the side effect of updating some dependencies.

I ran the tests on darwin. My machine is quite old so I had 8 failures due to a timeout. None of the failures look like they are due to the new dependencies.

Checking the pest repo, it seems there are some recent commits, however no sure date of when there could be a new release.

If this gets accepted, there is no need to do a new release, nixos can just target the new commit.

If you feel it's too much pain for not enough gain, no worries at all!

Co-authored-by: happysalada <raphael@megzari.com>
@happysalada
Copy link
Contributor

meilisearch has been integrated into nixpkgs, and after the proposed fixes have been merged to meilisearch, I've reverted the build system to a more traditional one. (see PR that also updates to 0.23.1)

I think we can safely close this issue.
Let me know if anything doesn't make sense.

@curquiza
Copy link
Member

Hello @happysalada! Thank you so much for this update ❤️
Feel free to open a new issue if something goes wrong!

@irevoire
Copy link
Member

I don't know if that can help you @happysalada but in the next release we are going to remove pest entirely since it's not maintained anymore.
Maybe you'll be able to simplify your installation script? 🙂
See this PR: meilisearch/milli#358

@happysalada
Copy link
Contributor

happysalada commented Oct 20, 2021

That PR is nice! Yeah nom makes much more sense!
I don't think this will influence packaging, but it's nice to know! Thank you for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

5 participants