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

Having a [patch] section when publishing is not an error #6535

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@jethrogb
Copy link
Contributor

jethrogb commented Jan 10, 2019

It is not necessary to throw an error when your Cargo.toml has a [patch] section.

  1. Cargo will normalize your Cargo.toml while packaging, which will remove the [patch] section.
  2. Even if the [patch] section were to somehow get to crates.io, it would be ignored when the crate is used since only the top-level crate's [patch] section is used.

The current behavior is quite annoying for crate maintainers who need a permanent [patch] section, for example, rand. This is the situation that leads to rand needing a permanent [patch] section:

image

Here, rand and rand_core are in the same repository, and rdrand is in a separate repository. When building rand as the top-level crate (e.g. in CI), the rand->rand_core path dependency will be used. This is of course a different crate than the crates.io rand_core used by rdrand. Therefore, when building rand as a top-level crate, you will always need to patch crates-io.rand to the path dependency. When building rand as a dependency, there are no issues, as the crates.io crates are used everywhere.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jan 10, 2019

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 10, 2019

This lgtm - I recently hit it and was annoyed. CI seems to be failing though.

r? @alexcrichton who implemented the check in the first place

@rust-highfive rust-highfive assigned alexcrichton and unassigned nrc Jan 10, 2019

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 10, 2019

I think it would be nice to have a test, and check if publish sends the information for the patched dependency or the non-patched one. I can help figure out how to write such a test if you need help, since it might be somewhat tricky to set up.

I don't have a strong opinion about the actual change. It does seem like it might be useful.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 10, 2019

Oh, and the documentation at https://github.com/rust-lang/cargo/blob/master/src/doc/man/cargo-publish.adoc will need to be updated, though I can take care of that if needed (the Makefile gives instructions for rebuilding which must be done manually).

@jethrogb

This comment has been minimized.

Copy link
Contributor

jethrogb commented Jan 11, 2019

@ehuss if you could give some pointers on how to test this, that would be great.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2019

Although not explicitly mentioned, it was the intention of the original RFC that this was disallowed. I think enough time has passed and we have enough experience that we can consider changing that decision, and I personally feel that with verification-on-build it's ok to do this.

I would like to get a team sign-off, however, because this is reversing an intentional decision.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 11, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Jan 11, 2019

What do you mean exactly by verification-on-build?

I think I would be ok with adding a flag that changes error on patch section to ignore patch section. (assuming good testing) This allows power users that know they need this to use it, while keeping the speed bump that says you are not testing what you are publishing.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 11, 2019

"cargo will remove" seems slightly inaccurate; doesn't that canonicalization happen on the crates.io server?

I'd be fine with this if cargo itself strips out the [patch] section before publishing.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 11, 2019

I'd be fine with this if cargo itself strips out the [patch] section before publishing.

It is stripped on the Cargo side when the .crate is crated. Here is the stripping logic.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 11, 2019

@ehuss Thanks! That answers my question.

Also, @alexcrichton, are you suggesting that cargo should check that the Cargo.toml of a downloaded .crate file doesn't contain a [patch] section? That seems like a good idea.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 11, 2019

It seems like it should work the same way I believe having path dependencies works - the build from cargo publish should ignore patches and path dependencies to test that this crate still builds with just the crates.io versions of all the dependencies. (I think that is what alex meant by "verification-on-build")

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 14, 2019

@Eh2406 ah by "verification-on-build" I actually mean "verification-on-publish" where I mean that when you cargo publish by default we'll build the crate as it shows up on crates.io, missing [patch] and all.

@joshtriplett ah no sorry that was a misspeak on my part, Cargo won't be performing any verification of downloaded crates.

@withoutboats I think you're right about path deps and such, in which case I actually think we should forgo a warning here at all and basically just delete the check

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 14, 2019

ah no sorry that was a misspeak on my part, Cargo won't be performing any verification of downloaded crates.

Could it? It seems appropriate to make sure that a crate can't accidentally pull in a random repo via [patch].

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 14, 2019

Cargo already doesn't use [patch] in dependencies at all, so I don't think we have much to gain about warning about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment