`cargo vendor` can't vendor `reqwest` dependency with different sources

[ancieg]

Summary

Can't vendor crates for v0.185.9.

Description

cargo vendor can't vendor reqwest dependency with different sources. Vendoring crates is required for reproducible builds.

Steps to reproduce:

1. git clone https://github.com/zed-industries/zed && cd zed
2. git checkout v0.185.9
3. cargo vendor
4. Encounter error:

error: failed to sync

Caused by:
  found duplicate version of package `reqwest v0.12.15` vendored from two sources:

  	source 1: registry `crates-io`
  	source 2: https://github.com/zed-industries/reqwest.git?rev=951c770a32f1998d6e999cef3e59e0013e6c4415#951c770a

Zed Version and System Specs

v0.185.9

[im-0]

It looks like cargo currently does not support this:

• https://github.com/rust-lang/cargo/issues/10310
• https://github.com/rust-lang/cargo/pull/13271

[im-0]

I was able to use cargo vendor with patched cargo: https://github.com/im-0/cargo/commit/c8524ebd42621131a6b776a69286c52f81f27c19 (this is just a https://github.com/rust-lang/cargo/pull/13271 but rebased on current cargo master).

[probably-neb]

Where is the need for cargo vendor coming from @ancieg? Right now I don't see why this is an issue for Zed

[im-0]

@probably-neb I use cargo vendor when building my own RPM packages for Fedora. Builders on copr are isolated from any network by default, thus cargo is unable to download any dependencies.

Yes, it is possible to enable internet access during build for specific repositories. But I thought that it will be nice to have a somewhat reproducible build process, so that any src.rpm is guaranteed to build again in the same environment.

[probably-neb]

I see. I think it's unlikely that we do anything about this, at least anytime soon. Glad the cargo patch is working for you. I recommend others maintain a custom patch for zed to fix this for themselves, or use the patched cargo (or just cargo if the PR is merged!)

[PedroHLC]

I've added reqwest = { git = "https://github.com/zed-industries/reqwest.git", rev = "951c770a32f1998d6e999cef3e59e0013e6c4415" } to [patch.crates-io] in Cargo.toml. And then called cargo update -p 'git+https://github.com/zed-industries/reqwest.git?rev=951c770a32f1998d6e999cef3e59e0013e6c4415#[email protected]' that ripped "reqwest 0.12.15 (registry+https://github.com/rust-lang/crates.io-index)" away.

But to my surprise, that fails to build with:

   Compiling reqwest v0.12.15 (https://github.com/zed-industries/reqwest.git?rev=951c770a32f1998d6e999cef3e59e0013e6c4415#951c770a)
error[E0599]: no method named `redirect` found for struct `async_impl::client::ClientBuilder` in the current scope
   --> /build/zed-editor-0.184.8-vendor-vendor/reqwest-0.12.15/src/blocking/client.rs:340:44

In my case, I can't bypass the offline restriction, or patch cargo. Suggestions?

[im-0]

@PedroHLC are you 100% sure that you can not use patched cargo? Patch is needed solely for cargo vendor. Then you can use normal unpatched cargo for cargo build.

[PedroHLC]

Unfortunately, it does not solely impact Cargo in my system. Since Nix likes to patch and hash all the sources, we use a utility called fetchCargoVendor. This utility also doesn't support duplicates (https://github.com/NixOS/nixpkgs/issues/359340).

EDIT: In parallel, Nixpkgs maintainers seem to be searching for a workaround https://github.com/NixOS/nixpkgs/pull/404938

[thomaseizinger]

But to my surprise, that fails to build with:

   Compiling reqwest v0.12.15 (https://github.com/zed-industries/reqwest.git?rev=951c770a32f1998d6e999cef3e59e0013e6c4415#951c770a)
error[E0599]: no method named `redirect` found for struct `async_impl::client::ClientBuilder` in the current scope
   --> /build/zed-editor-0.184.8-vendor-vendor/reqwest-0.12.15/src/blocking/client.rs:340:44

In my case, I can't bypass the offline restriction, or patch cargo. Suggestions?

This is because Zed's fork of reqwest actually renames this method in the async-client: https://github.com/seanmonstar/reqwest/commit/fd110f6998da16bbca97b6dddda9be7827c50e29#diff-0df5342f97493f9f55d6f43a14268f327ea9e791c7fdb8df6d88ec81f6baa721L1064-R1066

Hence, the blocking client which is built on top of the async one no longer compiles. The rename seems unnecessary though. Or if deemed required, it should be renamed also in the blocking module.

I am pretty sure the issue can be produced if you try to build https://github.com/zed-industries/reqwest/tree/0.12.8-per-request-redirect with all features. That could be submitted as a PR in order to fix that branch (which is what Zed internally depends on).

[ancieg]

Where is the need for cargo vendor coming from @ancieg? Right now I don't see why this is an issue for Zed

I need this for building RPM packages for ALT Linux Sisyphus. We have strict requirements for build sandbox: no internet, no root permissions, only system packages. This is needed to provide integrity of Sisyphus repository.

[ancieg]

The same issue with v0.186.9 and v0.186.11.

[thomaseizinger]

Where is the need for cargo vendor coming from @ancieg? Right now I don't see why this is an issue for Zed

I need this for building RPM packages for ALT Linux Sisyphus. We have strict requirements for build sandbox: no internet, no root permissions, only system packages. This is needed to provide integrity of Sisyphus repository.

Checkout https://github.com/NixOS/nixpkgs/pull/407051. It includes a patch that removes the duplicate dependency and then fixes the compile error by also renaming the function in the blocking client. This should allow you to make your build work until the problem is fixed in the reqwest fork of Zed.

[CathalMullan]

Did a little more digging to figure out where the 2nd reqwest version comes from. It's pulled in by the jsonschema feature resolve-http.

Removing that feature:

- jsonschema = "0.30.0"
+ jsonschema = { version = "0.30.0", default-features = false }

And regenerating the lockfile allows cargo vendor to run fine.

Replacing the default jsonschema retriever with a custom one that uses the internal version of reqwest could be an easy way to fix this issue for now.

EDIT: This would still hit the function rename bug mentioned above however, unless the async version of jsonschema was introduced.

[P1n3appl3]

Thanks for looking into this @CathalMullan. For the nixpkgs case it seems fine to wait for #387337 to land and use the patch for the meantime. Interestingly the zed package in our flake still builds successfully without patches, I assume because crane is using a different method than buildRustPackage to fetch the deps.

@im-0 is using --no-merge-sources enough to satisfy your use case for RPM packaging? If so I don't think there's any reason for us to land workarounds on the zed side for this given that people aren't generally using cargo-vendor other than for packaging.

[thomaseizinger]

@im-0 is using --no-merge-sources enough to satisfy your use case for RPM packaging? If so I don't think there's any reason for us to land workarounds on the zed side for this given that people aren't generally using cargo-vendor other than for packaging.

There is (almost) no workaround needed in Zed :)

All that needs to happen is that the fork of reqwest used by Zed needs to be updated to build with --all-features (that would be a good idea in general because it seems like it is an unfinished patch).

Once that is complete, you can replace reqwest using [patch.crates-io] across your entire dependency tree which should resolve this issue. YMMV but I think that is also a good idea in general as duplicated dependencies increase compile-times and in this particular case, will incentivize you to implement the feature in a backwards-compatible way which should also make upstreaming easier.

[im-0]

@P1n3appl3 --no-merge-sources is enough for me currently. However I would prefer to have this fixed in Zed. Currently Zed uses two slightly different copies of reqwest and this does not feel right.