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

prost fork is problematic when building buck2 with Nix #149

Closed
thoughtpolice opened this issue Apr 12, 2023 · 4 comments
Closed

prost fork is problematic when building buck2 with Nix #149

thoughtpolice opened this issue Apr 12, 2023 · 4 comments

Comments

@thoughtpolice
Copy link
Contributor

Commit afb3307 introduced a patched version of the prost and tonic dependencies to support boxing large fields in GRPC messages. This is a Very Good Idea since copying around massive structs is a common Rust pitfall and helps both improve performance and memory use. No objections there!

However, this broke my build of buck2 when using Nix in buck2-nix. In short the error is obtuse and looks like this, but I'm afraid I'm stuck at this point:

error: builder for '/nix/store/0xz81h8scjfgq0yfn3p13mn228wq9z8h-prost-0.11.6.drv' failed with exit code 101;
       last 10 log lines:
       > error: failed to load manifest for workspace member `/nix/store/wvlsz0nhhyb7kbc6794r6wc36k7ppzqh-prost-90b7e20/tests/single-include`
       >
       > Caused by:
       >   failed to load manifest for dependency `prost`
       >
       > Caused by:
       >   failed to read `/nix/store/prost/Cargo.toml`
       >
       > Caused by:
       >   No such file or directory (os error 2)
       For full logs, run 'nix log /nix/store/0xz81h8scjfgq0yfn3p13mn228wq9z8h-prost-0.11.6.drv'.

I believe this is due to the fact that the tests/single-include subdirectory of prost makes a reference to ../.. in order to find the prost package itself, since it's all part of a monorepo. But this won't work in Nix (using buildRustPackage), because it makes the build fully hermetic -- so such "outside project references" to other things are made invalid. There are some other packages that suffer from similar issues.

The normal version of prost works because the packages are downloaded from crates.io, so there is no git repo/monorepo involved at all, in that case.

At the moment, I can instead just manually revert the boxing patch, and thus also revert the changes to Cargo.toml, and that's what I've done, but it continues to get increasingly fiddly with every passing day (or week) and has some annoying edge cases in my build scripts to make it work.

Please note this is roughly stuck, I'm just filing to keep people in the loop; as of right now, this depends on tokio-rs/prost#802 and then hyperium/tonic#1252. Once the prost package has been updated (at minimum) I believe I will be able to work around the rest.

@thoughtpolice
Copy link
Contributor Author

thoughtpolice commented Apr 12, 2023

In an incredible twist of fate I just looked at the prost repository, and this commit is very relevant, from 2 days ago! My suspicion was wrong!

tokio-rs/prost@f63691b

So maybe this would actually work right now, if the prost patch could be rebased on top of it? Or this patch could just be applied to @krallin's fork, too...

@krallin
Copy link
Contributor

krallin commented Apr 12, 2023

We've been working on upstreaming this, but I'll take a look at whether I can rebase this

@thoughtpolice
Copy link
Contributor Author

Yeah, no rush on anybody, I can still hold onto the undo-boxing-patch if needed.

If a rebase can't work, just cherry-picking that one patch to your fork might get me somewhere.

@thoughtpolice
Copy link
Contributor Author

It looks like upstream accepted the patch to prost, and I can also say that tokio-rs/prost@f63691b also fixed the problem in the original comment, allowing Nix to build prost from the git repository -- combined, I can get my version of buck2 building again, I think!

So we still need to:

  • Wait for a release, and bump the versions, and,
  • Wait on tonic (but this isn't problematic for my purposes, it seems)

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 a pull request may close this issue.

2 participants