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

Failing to compile with locally vendored (patched) package #400

Open
mplanchard opened this issue Sep 27, 2023 · 15 comments
Open

Failing to compile with locally vendored (patched) package #400

mplanchard opened this issue Sep 27, 2023 · 15 comments

Comments

@mplanchard
Copy link

mplanchard commented Sep 27, 2023

Hello,

We have a situation where we've had to apply a patch to some upstream code, so we have vendored the code locally using cargo clone, and we are patching it in our Cargo.toml like so:

[patch.crates-io]
actix-http = { path = ".vendor/actix-http" }

We don't depend on actix-http directly: instead it's required by actix-web, which we do depend on.

However, when building a workspace package that depends on actix-web (by setting -p <package_name> in cargoExtraArgs), we get errors like:

error[E0432]: unresolved imports `actix_http::body`, `actix_http::HttpMessage`
  --> /nix/store/8zh0p4c9s5rwicvqynhb0m6hd5y5bar6-vendor-cargo-deps/c19b7c6f923b580ac259164a89f2577984ad5ab09ee9d583b888f934adbbe8d0/actix-web-4.4.0/src/lib.rs:77:22
   |
77 | pub use actix_http::{body, HttpMessage};
   |                      ^^^^  ^^^^^^^^^^^ no `HttpMessage` in the root
   |                      |
   |                      no `body` in the root

Essentially, everywhere actix_web tries to use actix_http, it seems to think it's an empty package with no symbols defined.

I have verified that the vendored code is making it into the source directory in the nix store.

I have also verified that the patch directive is making it into the Cargo.toml at the root of the source directory.

I have tried explicitly requiring actix-http = { path: "../../.vendor/actix-http" } in the project's Cargo.toml, but this doesn't seem to help.

Building the same package using buildRustPackage works without issues.

I'm continuing to debug, but if I'm missing something obvious or if you've got any suggestions for figuring out what's going on, please let me know.

@dpc
Copy link
Contributor

dpc commented Sep 27, 2023

Deps only derivation stripped this package out of the source code thinking it's the local one (because it is?). Look into how the deps only mkDummySrc works and issues before.

Quick fix to unblock would be probably to put it into a git repo, and patch it from there, until you have a better workaround.

@mplanchard
Copy link
Author

Thanks for the pointers! Unfortunately it would be a much larger change for us to push our vendored deps into a separate repo, so I'm trying to see if I can use mkDummySrc to get this working.

It's not immediately obvious to me though how a mkDummySrc-derived result should be passed into buildPackage. This is what I'm trying, simplified a bit:

        packages.my-pkg =
          let
            baseArgs = {
              pname = "my-pkg";
              version = "${revision}";
              cargoExtraArgs = "-p my-pkg";
              doCheck = false;
            };
          in
          craneLib.buildPackage (baseArgs // {
            cargoArtifacts = craneLib.buildDepsOnly (baseArgs // {
              installCargoArtifactsMode = "use-zstd";
              installPhase = "prepareAndInstallCargoArtifactsDir";
              dummySrc = craneLib.mkDummySrc {
                src = craneLib.cleanCargoSource (craneLib.path ./.);
                extraDummyScript = ''
                  set -exuo pipefail
                  cp -rf ${./.vendor} $out/.vendor
                '';
              };
            });
          });

This fails in the same way as before, and looking at the log output, I'm seeing the message: cargoArtifacts not set, will not reuse any cargo artifacts.

Am I going in the right direction with the extraDummyScript bit to copy in the locally vendored dependencies? If so, what am I doing wrong to get buildPackage to use that?

Thanks for any help you can provide.

@dpc
Copy link
Contributor

dpc commented Sep 27, 2023

It's probably best to split craneLib.buildPackage into two derivations first: buildDepsOnly + buildPackage, which is being done implicitly normally. It should make debugging easier at least, and maybe there are some weird corner cases. example I have at hand with mkDummySrc

@mplanchard
Copy link
Author

Thanks for your continued help! I managed to get the files included with something like this (the --no-target-directory thing turned out to be critical):

          let
            commonInputs = [ pkgs.openssl ];
            commonArgs = {
              src = dummySrc;
              nativeBuildInputs = [
                pkgs.lld
                pkgs.pkg-config
              ] ++ commonInputs;
              pname = "my-pkg";
              version = "${revision}";
              preInstall = "echo 'hello'";
              SQLX_OFFLINE = "true";
              OPENSSL_DIR = "${pkgs.openssl.dev.outPath}";
              OPENSSL_LIB_DIR = "${pkgs.openssl.out.outPath}/lib";
            };
            dummySrc = craneLib.mkDummySrc {
              src = craneLib.path ./.;
              extraDummyScript = ''
                set -exuo pipefail
                cp -rf --no-target-directory ${./.vendor} $out/.vendor
                ls $out/.vendor/actix-http/src
              '';
            };
            cargoArtifacts = craneLib.buildDepsOnly (commonArgs // {
              inherit dummySrc;
            });
          in
            craneLib.buildPackage (commonArgs // {
              inherit cargoArtifacts;
              cargoExtraArgs = "-p my-pkg";
              doCheck = false;
              buildInputs = commonInputs;
            });

This is maybe a separate issue, but I'm now running into an error after the check phase:

checkPhase completed in 4 minutes 23 seconds
@nix { "action": "setPhase", "phase": "installPhase" }
installing
/nix/store/xyf78lzdkc2vlrm939a192hsfzli31i0-installCargoArtifactsHook/nix-support/setup-hook: line 65: cargoArtifacts: unbound variable

My actual preInstall is not echo hello, but could the presence of the preInstall hook be causing this?

@dpc
Copy link
Contributor

dpc commented Sep 27, 2023

if [ -n "${cargoArtifacts}" ] && [ -d "${cargoArtifacts}/target" ]; then

It might be due to that set -exuo pipefail you've added.

@mplanchard
Copy link
Author

Oh, good catch!

@mplanchard
Copy link
Author

mplanchard commented Sep 28, 2023

Okay, this has gotten me pretty close. Now I get a bunch of warnings about what seems to be an unrelated duplicate local crate, and a permissions error related to an rlib:

++ command cargo build --release --message-format json-render-diagnostics -p my_pkg
warning: output filename collision.
The lib target `my-library` in package `my-library v0.1.0 (/build/source/lib/rs/my-library)` has the same output filename as the lib target `my-library` in package `my-library v0.1.0 (/build/source/lib/rs/my-library)`.
Colliding filename is: /build/source/target/release/deps/libmy-library.so
The targets should have unique names.spec_proxy
Consider changing their names to be unique or compiling them separately.
This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.
warning: output filename collision.
The lib target `my-library` in package `my-library v0.1.0 (/build/source/lib/rs/my-library)` has the same output filename as the lib target `my-library` in package `my-library v0.1.0 (/build/source/lib/rs/my-library)`.
Colliding filename is: /build/source/target/release/deps/libmy-library.so.dwp
The targets should have unique names.
Consider changing their names to be unique or compiling them separately.
This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.
warning: output filename collision.
The lib target `my-library` in package `my-library v0.1.0 (/build/source/lib/rs/my-library)` has the same output filename as the lib target `my-library` in package `my-library v0.1.0 (/build/source/lib/rs/my-library)`.
Colliding filename is: /build/source/target/release/deps/libmy-library.rlib
The targets should have unique names.
Consider changing their names to be unique or compiling them separately.
This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.
   Compiling my-library v0.1.0 (/build/source/lib/rs/my-library)
error: output file /build/source/target/release/deps/libmy-library.rlib is not writeable -- check its permissions

Do you think this is related to my dummySrc shenanigans? This crate does have both cdylib and rlib in its crate types, but I was under the impression from the API docs that rlib builds would be ignored.

I don't get the same error if I invoke craneLib.buildPackage on the library in question directly, only for this build of the package that depends on it.

This isn't one of the packages that's in the vendor directory, just a normal dependent library in the workspace.

@dpc
Copy link
Contributor

dpc commented Sep 28, 2023

The warnings are ... warnings. So it's only the

error: output file /build/source/target/release/deps/libmy-library.rlib is not writeable -- check its permissions

Put some debug code in the bash hooks before this step and verify the permissions and why it's not writable.

@dpc
Copy link
Contributor

dpc commented Sep 28, 2023

Hmmm..

You need

              installCargoArtifactsMode = "use-zstd";

in all args (or use some common args pattern to merge them into everything).

@mplanchard
Copy link
Author

Ah, that did it! Finally got a build to complete. Thanks so much for all your help 🎉

Very excited to be able to do much better caching of our dependency tree in CI

@mplanchard
Copy link
Author

mplanchard commented Sep 28, 2023

Just for the sake of anyone else who finds this issue, the working version is:

packages.my-pkg =
  let
    revision = self.rev or "dev";
    commonInputs = [ pkgs.openssl ];
    dummySrc = craneLib.mkDummySrc {
      src = craneLib.path ./.;
      extraDummyScript = ''
        set -exuo pipefail
        cp -rf --no-target-directory ${./.vendor} $out/.vendor
      '';
    };
    commonArgs = {
      src = dummySrc;
      installCargoArtifactsMode = "use-zstd";
      nativeBuildInputs = [
        pkgs.lld
        pkgs.pkg-config
      ] ++ commonInputs;
      buildInputs = commonInputs;
      cargoExtraArgs = "-p my-pkg";
      version = "${revision}";
      doCheck = false;
      SQLX_OFFLINE = "true";
      OPENSSL_DIR = "${pkgs.openssl.dev.outPath}";
      OPENSSL_LIB_DIR = "${pkgs.openssl.out.outPath}/lib";
    };
    cargoArtifacts = craneLib.buildDepsOnly (commonArgs // {
      inherit dummySrc;
      pname = "my-pkg-deps";
    });
  in
    craneLib.buildPackage (commonArgs // {
      inherit cargoArtifacts;
      src = craneLib.cleanCargoSource (craneLib.path ./.);
      pname = "my-pkg";
    });

@dpc
Copy link
Contributor

dpc commented Sep 28, 2023

Very excited to be able to do much better caching of our dependency tree in CI

If you're interested in ultra-optimized CI, let's stay in touch . I recommend keeping your eye on Flakebox, try the upcoming incremental zstd change and some discussion about it.

There's are so many rarely employed ways to optimize Rust building both in the dev shell and in the CI, and I'd love for them to become more common place.

@mplanchard
Copy link
Author

I am absolutely interested, yes. We've got a few fairly large Rust projects, local tools written in Rust, and Rust WASM builds incorporated into our FE stack, so we do a lot of building, both locally and in CI. We've also got cross-compilation needs, since a lot of our devs are on M-series macs and still need to be able to build linux binaries and images.

We've so far mostly used nix just for consistent dev environments and docker image builds, but I'm just switching us over from niv to flakes, largely to be able to use projects like crane so that we can build more quickly and avoid spurious rebuilds.

Thanks for all the links! We're using lld on our Linux boxes, which gave us a decent bump over the default. I'll have to give mold a try to see if that gives us another bump, and I'll check out that compress-debug-sections argument too. I'll also read up on and keep an eye on Flakebox

@dpc
Copy link
Contributor

dpc commented Sep 28, 2023

I am absolutely interested, yes. We've got a few fairly large Rust projects, local tools written in Rust, and Rust WASM builds incorporated into our FE stack, so we do a lot of building, both locally and in CI. We've also got cross-compilation needs, since a lot of our devs are on M-series macs and still need to be able to build linux binaries and images.

At Fedimint we solved all of these and some: https://github.com/fedimint/fedimint/actions/runs/6341294780

https://twitter.com/dpc_pw/status/1704639128107930056

@mplanchard
Copy link
Author

1.5 years on and I had totally forgotten about this, was just bitten by it again with a locally vendored package.

I'm gong to reopen, b/c even though there is a workaround, it would be nice if crane handled patches more cleanly in this case.

@mplanchard mplanchard reopened this Mar 24, 2025
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

No branches or pull requests

2 participants