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

Support for vendoring on a single platform #7058

Open
nipunn1313 opened this issue Jun 21, 2019 · 17 comments
Open

Support for vendoring on a single platform #7058

nipunn1313 opened this issue Jun 21, 2019 · 17 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-vendor S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@nipunn1313
Copy link
Contributor

See alexcrichton/cargo-vendor#70

Would be nice to be able to vendor on linux without pulling in windows-specific dependencies.
This would still be something we would use.

@ehuss ehuss added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-vendor labels Jun 21, 2019
@kvark
Copy link

kvark commented Oct 4, 2019

@alexcrichton this is actually hurting us a bit with Gecko as well. I'm trying to vendor https://github.com/grovesNL/spirv_cross which has a wasm-specific dependency:

[target.wasm32-unknown-unknown.dependencies]
wasm-bindgen = "0.2.33"
js-sys = "0.3.10"

The difference with the original issue is that we'd need a set of targets to filter on - the set Gecko needs to be building for.

@alexcrichton
Copy link
Member

It's worth copying over the context of alexcrichton/cargo-vendor#70 (comment) here I think. tl;dr; this is, I believe, blocked on #5133

Certainly a feature I'd like to implement! Supporting this will be pretty difficult though, unfortunately. Cargo currently requires that the lock file is platform-independent, meaning that the lock file (and generating the lock file) always has to locate all crates for all possible platforms. This means that if you were to remove the winapi folder, for example, then a lock file would no longer be correctly generated.

There's a tracking issue for this at #5133 which is a possible way to pass a flag to Cargo and say "please generate a minimal lock file", and with that we can safely delete all the dependencies for other platforms.

@thefallentree
Copy link

+1 this would be very useful.

@est31
Copy link
Member

est31 commented Sep 23, 2020

An alternative to solve this issue with platform specific lockfiles would be to generate empty stubs for crates left out from the build, just with enough information for cargo to make sense of the passed lockfile.

To give a different way of looking at this proposal: this method is already implemented for unvendored crates.io crates in the form of the index. Stub Cargo.tomls would be nothing else than extracting those parts of the index that are relevant so that the vendor setting doesn't need to use the index. Only the format would be different from the index's json format and use Cargo.toml instead.

The advantage: it would only require functional changes in cargo-vendor but not cargo itself, and especially not require complicated resolver changes.

I think from a practical standpoint, this approach could solve the main objections of people, weird files* included by dependencies and the size bloat. It might seem like a hack at first sight, but as I've outlined above, the same thing is done with the index already.

*: Note that I wouldn't be unhappy if raw dylibs finally got implemented so that winapi can drop those files

@ehuss
Copy link
Contributor

ehuss commented Oct 20, 2020

@Eh2406 wrote some details about this at #8723 (comment).

@avindra
Copy link

avindra commented Dec 28, 2020

This affects https://github.com/denoland/deno too. Here is the layout of Deno's vendor tarball created on Linux. >20% of the size of the vendor directory comes from Winapi, which is amazing considering it also contains the v8 engine and portions of Chrome's bespoke build system.

image

@avindra
Copy link

avindra commented Dec 28, 2020

Sharing a workaround to remove the bulk of the bulky .a files from winapi*gnu. I am still able to compile Deno on linux after removing these files:

rm -fr vendor/winapi*gnu*/lib/*.a

Those files account for the bulk of the contribution from the winapi folders (they are the orange boxes in the two largest yellow regions of the previously attached screenshot).

travier added a commit to travier/rpm-ostree that referenced this issue Mar 8, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
travier added a commit to travier/afterburn that referenced this issue Mar 8, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
travier added a commit to travier/ssh-key-dir that referenced this issue Mar 8, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
cgwalters pushed a commit to coreos/rpm-ostree that referenced this issue Mar 8, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
travier added a commit to travier/coreos-installer that referenced this issue Mar 17, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
travier added a commit to travier/zincati that referenced this issue Mar 17, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
travier added a commit to travier/zincati that referenced this issue May 9, 2022
Workaround to remove Windows specific binaries that we don't need from
the vendored sources.

See: rust-lang/cargo#7058
@cgwalters
Copy link

An alternative to solve this issue with platform specific lockfiles would be to generate empty stubs for crates left out from the build, just with enough information for cargo to make sense of the passed lockfile.

OK, I have a working PoC of this that gives me a Linux-only vendor/ directory; lots more to do of course:

https://github.com/cgwalters/cargo-vendor-filterer

@cgwalters
Copy link

cgwalters commented Jun 22, 2022

OK 🆕 A lot more work, bugfixes, new features (configurable via Cargo.toml metadata, removing files like vendored C libraries, writing reproducible tarballs), and the first release 0.5 is up: https://crates.io/crates/cargo-vendor-filterer - cargo install cargo-vendor-filterer --version ^0.5.

Definitely looking for feedback! I'm going to make a push to use it for some of our projects that only target Linux. Tentatively, let's have this live as a separate wrapper for now, iterate on it with interested users, and then try to land this in the main cargo vendor at some point?

@kgrech
Copy link

kgrech commented Nov 22, 2022

I am happy to commit to contribute this feature. I've tried to hack around and it looks like I am able to take the Graph struct used by cargo tree and adopt it to be used by cargo vendor command. I think it would also solve issue #9980 and #7065.

What do you think about the idea of reusing the graph from tree command with cargo vendor command? This commit is what I have so far. It needs a lot of polishing, but it is the illustration of the implementation idea

I am happy to commit to contribute it as the PR, but it is my first contribution to cargo. I've read the proccess and I do not see Feature accepted label here.

Do we need the PRC for this feauture? It looks straightforward for me, we just need to support same flags as cargo tree in the similar way.

Could I get confirmation that cargo community would be happy to accept this feature assuming the PR quality is up to the bar? I would need minimal support, mainly to discuss if we would like to change the current default behaviour of cargo vendor

CC @ehuss @alexcrichton @Eh2406 @weihanglo @Zymlex

UPD: Looks like I understand the bigger problem now :( Even I can vendor only required dependencies, cargo build still fails after, because some dependencies are not available (even they are not going to be used during this build). Now the comments above about Cargo.lock being single source of truth started making sense.

Now I understand that the problem is not with cargo vendor command, but with cargo build. If I simply remove winapi, I get:

cargo build --target=x86_64-unknown-linux-gnu
error: no matching package found
searched package name: `winapi`
perhaps you meant:      wasi
location searched: registry `crates-io`
required by package `async-io v1.10.0`
    ... which satisfies dependency `async-io = "^1.0.1"` (locked to 1.10.0) of package `async-std v1.12.0`

Which does not make much sense, but it is how it is

Is there any workaround for this except from making something like platform specific Cargo.lock?

@weihanglo
Copy link
Member

Unfortunately, no. Not at this moment 😞. See #8723 (comment) for ideas from Eh2406. Also #10718 a semi-related issue about platform-specific cargo metadata.

From my point of view, reusing code from cargo tree might not be the best approach, as cargo tree doesn't go through the whole build process. But the idea of flag --edges or a more could be a good start.

@WhyNotHugo
Copy link

If a package could declare "I don't work on X platform", then lockfile generation could exclude all dependencies for that platform. This would avoid the whole "lockfile won't match" issue mentioned above.

For a package that won't work on Windows, being able to mark it as such would avoid the hundreds of megabytes typically pulled in for tiny projects.

I'm not sure if this cover 100% of the cases here. Is there some scenario where a package does work on a platform but you'd want to avoid vendoring dependencies for that platform?

@est31
Copy link
Member

est31 commented Apr 3, 2023

Is there some scenario where a package does work on a platform but you'd want to avoid vendoring dependencies for that platform?

Linux packagers generally only care about having enough sources for it to work on their specific distro. It feels to me that this is the largest group of people in this thread. They just want something that passes the desert island test.

But IMO, this would ideally be resolved by having stubs or other data that cargo can use to support the crates during resolution but error during build time. Platform specific resolution is a bad path to go down on. For example, consider a workspace where you have crate A that is platform independent, and crate B that is platform specific. Crate A depends on crate Z and B depends on A. Crate Z depends on various crates depending on the target OS. Now, you'd have two copies of crate Z in Cargo.lock: one version where A is the leaf crate, and one version where B is the leaf crate.

@WhyNotHugo
Copy link

That's a good point.

On a related note, it would also be useful to be able to mark a crate as "not available for target X". For a few of my specific use-cases, the code won't run (I'm not sure if it even builds) on Windows, since it relies on a bunch of OS features not available.

Being able to mark "ignore target windows for this crate" would also force the lockfile to ignore all windows-only transitive dependencies, since they are never needed.

However, this won't cover all other cases regarded here.

@WhyNotHugo
Copy link

WhyNotHugo commented Apr 3, 2023

Platform specific resolution is a bad path to go down on. For example, consider a workspace where you have crate A that is platform independent, and crate B that is platform specific. Crate A depends on crate Z and B depends on A. Crate Z depends on various crates depending on the target OS. Now, you'd have two copies of crate Z in Cargo.lock: one version where A is the leaf crate, and one version where B is the leaf crate.

Completely agree here. What are your thoughts on (optionally) declaring platforms at a workspace level?

Either by exclusion ("works everywhere except X") or inclusion ("only works on X and Y"). The lockfile will then reflect that.

@epage
Copy link
Contributor

epage commented Apr 18, 2023

If a package could declare "I don't work on X platform", then lockfile generation could exclude all dependencies for that platform. This would avoid the whole "lockfile won't match" issue mentioned above.

We have #9406 for force-target but instead this would be a required-target (somewhat analogous to required-features), see #6179.

Another benefit to this solution is it allows you to run cargo check --workspace and skip building the packages not relevant for the current platform. cargo has added a workspace but we can't run cargo check --workspace because of this (#11987).

Ideally, each package can declare its required target. We'd need to pass this information down as we recurse and take the intersection of a crate and all of its dependents required targets. This would add a bit of bookkeeping to the resolver.

We would need to decide what this also means if a crate doesn't specify required-target or specifies more than its dependencies, what happens?

One of the problems with this is the natural way of defining the manifest schema is for required-target to accept cfg syntax. The problem is we'd then need to compare required-targets cfg with the target-dependencies cfg. The only way to do this is if we have a master list of all targets and run it through both cfg and then take the intersection of that.

Questions for cfg

  • Can cargo get the complete target list?
  • If new targets are added/removed with new rust versions, will this change the contents of the Cargo.lock file?

@weihanglo weihanglo added the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label Jul 18, 2023
facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Aug 23, 2023
Summary:
Imported from https://github.com/Boscop/web-view/ commit 82d7cbce6228b1a964673cc0f22944ad808eab42.
Non-mac version is dropped (on Windows `edge.exe --app=url` can be a decent
alternative, on Linux it can be challenging to get dependencies ready).

We didn't import it to the repo-wide third-party library because `reindeer`
uses `cargo vendor` and `cargo vendor` cannot skip (hard-to-build) Linux
dependencies even if the crate `webview-sys` is only referred for macOS.
See rust-lang/cargo#7058

Reviewed By: sggutier

Differential Revision: D48531141

fbshipit-source-id: 4a087e83deae3764edcad5725d1a93d28d8ff6a8
@dravenk
Copy link

dravenk commented Jan 12, 2024

The winapi and windows-sys crates like a virus is everywhere. I write some code run on linux platform only, but cargo vendor download the whole windows dependencies close 300M. This completely unused code takes up more than 90% of my codebase. It's a bad hell. This will be a very useful feature for writing small tools in rust.

wdoekes added a commit to ossobv/natsomatch that referenced this issue Feb 15, 2024
Also, monitor rust-lang/cargo#7058 so we can
trim this down.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-vendor S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests