facebook / buck2

Build system, successor to Buck
https://buck2.build/
Apache License 2.0
3.53k stars 215 forks source link

prost fork is problematic when building buck2 with Nix #149

Closed thoughtpolice closed 1 year ago

thoughtpolice commented 1 year ago

Commit afb3307b6141d87b88f1de9f7f74bd9791ad2293 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 commented 1 year ago

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!

https://github.com/tokio-rs/prost/commit/f63691b6570297734f298ebe8ca54adad2ed629b

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 commented 1 year ago

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

thoughtpolice commented 1 year ago

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 commented 1 year ago

It looks like upstream accepted the patch to prost, and I can also say that https://github.com/tokio-rs/prost/commit/f63691b6570297734f298ebe8ca54adad2ed629b 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: