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

RTS build: don't vendor Rust deps #2829

Closed
wants to merge 3 commits into from
Closed

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Oct 6, 2021

Vendoring dependencies is causing a lot of problems in #2761 and other
PRs, because cargo vendor currently has a bug and cannot vendor
standard dependencies (deps of alloc and std, see
rust-lang/wg-cargo-std-aware#23).

For a long time I thought vendoring is a necessity and tried to work
around this issue by vendoring dependencies manually, or using
https://github.com/nix-community/naersk.

However, it turns out vendoring is not necessary, and we can download
dependencies in build step just fine. I also don't see any advantages of
vendoring the dependencies. So in this PR we remove vendoring.

Unblocks #2761.

Vendoring dependencies is causing a lot of problems in #2761 and other
PRs, because `cargo vendor` currently has a bug and cannot vendor
standard dependencies (deps of alloc and std, see
rust-lang/wg-cargo-std-aware#23).

For a long time I thought vendoring is a necessity and tried to work
around this issue by vendoring dependencies manually, or using
https://github.com/nix-community/naersk.

However, it turns out vendoring is not necessary, and we can download
dependencies in build step just fine. I also don't see any advantages of
vendoring the dependencies. So in this PR we remove vendoring.

Unblocks #2761.
@osa1 osa1 requested a review from nomeata October 6, 2021 06:20
@osa1
Copy link
Contributor Author

osa1 commented Oct 6, 2021

Sigh... This works fine locally but on CI fails because I think it can't download stuff in build step?

I don't understand why it can download stuff in preBuild step (for vendoring) but not in build step..

@osa1
Copy link
Contributor Author

osa1 commented Oct 6, 2021

Weird, CI passed on macos, but not on Ubuntu. Maybe a problem with Ubuntu CI configuration?

(I've restarted CI)

@nomeata
Copy link
Collaborator

nomeata commented Oct 6, 2021

Downloading stuff in nix builds is of course a no-go; how else would you achieve hermeticity - the content of the URL could change, making the build nonreproducable.

It may appear to work in some system configurations that don't sandbox the build (e.g. on Darwin CI, it seems), but it (rightfully) will not work on properly sandboxed builds. There is a reason naersk etc exists and does this step where it downloads the dependencies first to a “fixed output derivation”, where the content (hash) is declared ahead of time, and which therefore are allowed to do unsafe things.

@osa1
Copy link
Contributor Author

osa1 commented Oct 6, 2021

Downloading stuff in nix builds is of course a no-go; how else would you achieve hermeticity - the content of the URL could change, making the build nonreproducable.

We already download stuff from crates.io today. It just happens in preBuild step rather than build step. If a crate contents change in crates.io (which I think should never happen, and checksums in Cargo.lock are to check this) we will have all of the same problems in any case.

Why is it a problem to download in build step but not in preBuild step?

@nomeata
Copy link
Collaborator

nomeata commented Oct 6, 2021

I would be very surprised if that makes a difference, these steps are just different parts of essentially one big shell script. . Are you sure it's not the difference between a normal build and a fixed-output build (like the normal vendoring build step)?

@osa1
Copy link
Contributor Author

osa1 commented Oct 6, 2021

Ah, you're right, preBuild script is called in build: https://github.com/NixOS/nixpkgs/blob/409290b0ea00767b8632a93701f8edb82760cfaa/pkgs/stdenv/generic/setup.sh#L1051:L1077 so there shouldn't be any difference about sandboxing between those steps.

I'm a bit confused about why we are able to download packages for vendoring, but not for building. The rustDeps derivation

      rustDeps = nixpkgs.rustPlatform-nightly.fetchCargoTarball {
        name = "motoko-rts-deps";
        src = subpath ./rts;
        sourceRoot = "rts/motoko-rts-tests";
        sha256 = "0jyp3j8n5bj5cy1fd26d7h55zmc4v14qc2w8adxqwmsv5riqz41g";
        copyLockfile = true;
      };

is used in preBuild step:

      preBuild = ''
        ...
        unpackFile ${rustDeps}
        ...
      '';

So downloading the packages should happen in preBuild step, which is part of the build step as shown in the link above. So why does downloading for vendoring work, but downloading for building does not?

@nomeata
Copy link
Collaborator

nomeata commented Oct 6, 2021

Both are builds (i.e. a derivation, producing a store path when built). But the vendoring build has, as part of it's build instructions, the hash of the result. This is the cargo hash we have to manually update. The build would fail if that doesn't match after running the build steps, and therefore the build is reproducible by construction. And so these fixed-output derivations are allowed to access the internet, in contrast to the normal derivations.

@nomeata
Copy link
Collaborator

nomeata commented Oct 6, 2021

@osa1
Copy link
Contributor Author

osa1 commented Oct 6, 2021

But the vendoring build has, as part of it's build instructions, the hash of the result

OK, this makes sense.

I wonder if naersk uses cargo vendor. If it does then we can't use it to work around the cargo vendor bug.

@osa1 osa1 closed this Oct 6, 2021
@osa1 osa1 deleted the osa1/dont_vendor_rust_deps branch October 6, 2021 09:37
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