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

Do not filter against crates io #39

Merged
merged 1 commit into from Dec 1, 2022
Merged

Do not filter against crates io #39

merged 1 commit into from Dec 1, 2022

Conversation

kgrech
Copy link
Contributor

@kgrech kgrech commented Nov 29, 2022

This is how my Cargo.toml looks like:

[package]
name = "my_package"
version = "0.1.0"
edition = "2021"

[dependencies]
thiserror = "1.0.30"
tracing = "0.1"
# Fix for https://github.com/Absolucy/tracing-oslog/pull/3
tracing-oslog = { git = "https://github.com/kgrech/tracing-oslog", branch = "main" }
tracing-core = "0.1"
tracing-subscriber = "0.3"

[dev-dependencies]
libc = "0.2"

You can see that tracing-oslog is not fetched from crates-io, but it is fetch from github. I would sill like to vendor it so that I can build my project without connection to internet.

However right now it is not vendored because of the .filter(|source| source.is_crates_io()) line.

I am making an assumption that the idea of this if is not to vendor local dependencies. However if it is a local dependency, then source for it is already None and there is no need to have a filter on top of it.

My assumption could be very well wrong. I can imagine the situation where some crates are pulled from local corporate git system and some are pulled from the internet. For such case, skipping .filter(|source| source.is_crates_io()) makes more sense, but it still looks far from perfect to me as it would fail as soon as you would like to make a fork of some github package on github.

If the intention was to support the second case, would not it be better idea to have allowlist and blocklist support for the source strings in form of regular expressions? Or faster alternative could to make filtering configurable by passing something like --skip-non-crates-io

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks sane to me!

I have a vague recollection that I wrote that logic to try to match a behavior of cargo vendor, but indeed current cargo vendor seems to agree with this. And skimming over the current sources as well as https://github.com/rust-lang/cargo/commits/7b9069e8f928290cc54a2d8f27fc41095a68f67e/src/cargo/ops/vendor.rs I'm not seeing any recent changes here.

@cgwalters cgwalters merged commit 88abc45 into coreos:main Dec 1, 2022
@cgwalters
Copy link
Member

Would be nice in the future to add a test case; one thing we could do pretty easily in CI here is "reverse dependency testing" - if you have a public project that is configured this way, we could check it out and verify it vendors.

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.

None yet

2 participants