Implement the [patch] section of the manifest #4123

alexcrichton · 2017-06-05T14:34:54Z

This is an implementation of RFC 1969 which adds a new section to top-level
manifests: [patch]. This section allows you to augment existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

rust-highfive · 2017-06-05T14:35:04Z

r? @brson

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

alexcrichton · 2017-06-05T14:35:05Z

r? @matklad

alexcrichton · 2017-06-05T14:35:42Z

Note that this shouldn't actually get merged until rust-lang/rfcs#1969 is merged, and I still need to update src/doc/*.md to document this new feature over [replace] as well. Other than that though the implementation should be ready for review!

bors · 2017-06-05T15:29:13Z

☔ The latest upstream changes (presumably #4118) made this pull request unmergeable. Please resolve the merge conflicts.

matklad

I've just given a really quick skim, and looks like all the tests test replace scenario, where we augment with the same version we already have. Would be nice to have some tests for second and third scenario from the RFC!

matklad · 2017-06-05T20:40:30Z

tests/augment.rs

+
+#[test]
+fn nonexistent() {
+    Package::new("baz", "0.1.0").publish();


Hm, baz is not mentioned anywhere else in the tests?

Oh this is used to route registry requests (e.g. updating the index) to the local version and not crates.io itself.

alexcrichton · 2017-06-06T14:52:05Z

Sure yeah, pushed up some more tests. Some drafts of docs are up but they still need serious reworking (this is just whatever work I had done from awhile ago)

matklad · 2017-06-07T20:12:33Z

src/cargo/core/manifest.rs

@@ -28,13 +29,15 @@ pub struct Manifest {
    profiles: Profiles,
    publish: bool,
    replace: Vec<(PackageIdSpec, Dependency)>,
+    augment: HashMap<Url, Vec<Dependency>>,


Hm, specifying augment as a Dependency (that is, a constraint on possible versions) does not feel semantically right. What does it mean to augment a registry with foo 1.0.*?

Should we add a constraint that you must explicitelly add a =x.y.z constraint to the augment? This seems better from the theoretical pov, but I don't see any real benefits except perhaps for the better readability:

[augment.crates-io] xml-rs = { git = "https://github.com/aturon/xml-rs", branch = "servo-fix" }

vs

[augment.crates-io] xml-rs = { version = "0.9.2", git = "https://github.com/aturon/xml-rs", branch = "servo-fix" }

That is, I am a bit unsure about

Cargo will unconditionally resolve all entries in the [augment] section to precise dependencies, encoding them all in the lock file whether they're used or not.

section of the RFC. Should cargo really resolve entries in augment? What if it finds several versions of [augment] (it's not possible with git source, but may be possible with some theoretical other), which one should it choose?

Ah, I see that we just bail if augment resolves to more than one summary!

Yeah the intention here was that the syntax of this section is the same as [dependencies], so we'd just store it as a vanilla Dependency. Then it only makes sense to augment with git sources or path sources, which only ever return one summary anyway, so it'd suffice for that.

matklad · 2017-06-07T20:16:21Z

src/cargo/core/workspace.rs

+    /// Returns the root [augment] section of this workspace.
+    ///
+    /// This may be from a virtual crate or an actual crate.
+    pub fn root_augment(&self) -> &HashMap<Url, Vec<Dependency>> {


Am I correct that augment is only allowed in the root of the workspace? We should probably enforce this.

It is, yeah. I'll look to adding enforcement.

matklad · 2017-06-07T20:27:43Z

src/cargo/core/registry.rs

+            format!("failed to resolve augments for `{}`", url)
+        })?;
+
+        *self.augments.entry(url.clone()).or_insert(Vec::new()) = deps;


Looks suspicious... It should be either augments.insert(url, deps) or augments.entry().or_insert().extend(deps).

I feel like I originally had this as extend and later changed it to this. I'll update the code.

matklad · 2017-06-07T20:51:14Z

src/cargo/core/registry.rs

+            // name/version. Note that we don't use `dep.matches(..)` because
+            // the augmentations, by definition, come from a different source.
+            // This means that `dep.matches(..)` will always return false, when
+            // what we really care about is the name/version match.


Let's add something like match_ignore_source to dependency, just to keep all related logic in one place? It's interesting that we need something similar for overrides (only_match_name thing), but the implementation there uses a different strategy.

Yeah old overrides are... odd...

In any case though the suggestion sounds good!

matklad · 2017-06-07T21:01:56Z

src/cargo/core/registry.rs

+                    // check its results. We don't actually use any of the summaries it
+                    // gives us though.
+                    (Some(override_summary), Some(source)) => {
+                        let mut n = 0;


I think in this branch we should bail with an error if !augments.is_empty().

matklad · 2017-06-07T21:05:01Z

src/cargo/core/registry.rs

+        // Finally we check to see if any registered augments correspond to
+        // this dependency.
+        let v = augments.get(dep.source_id().url()).and_then(|vec| {
+            vec.iter().find(|s| {


We could add a sanity check here that we find at most one matching augment.

matklad · 2017-06-07T21:18:21Z

src/cargo/core/resolver/encode.rs

@@ -17,6 +17,12 @@ pub struct EncodableResolve {
    /// `root` is optional to allow forward compatibility.
    root: Option<EncodableDependency>,
    metadata: Option<Metadata>,
+    augment: Option<Augment>,


I think we can use null object pattern and store augment: Augment, using an empty augment instead of None. Serialization can be tweaked separately:

#[serde(default)] #[serde(skip_if = "Augment::is_empty")] augment: Augment

matklad · 2017-06-07T21:19:41Z

src/cargo/core/resolver/encode.rs

@@ -325,10 +344,27 @@ impl<'a, 'cfg> ser::Serialize for WorkspaceResolve<'a, 'cfg> {
            Some(root) if self.use_root_key => Some(encodable_resolve_node(&root, self.resolve)),
            _ => None,
        };
+
+        let augment = if self.resolve.unused_augments().len() == 0 {


Could use is_empty().

Probably irrelevant if we go with null object here :)

matklad · 2017-06-07T21:27:09Z

src/cargo/ops/resolve.rs

+            }
+        };
+        let augments = augments.iter().map(|dep| {
+            let unused = previous.unused_augments();


Hm, what if previous contains some old augments, which are no longer present in the current Cargo.toml?

I think these'll just get dropped later, but I've added a test to assert this as well.

matklad · 2017-06-07T21:31:30Z

src/doc/manifest.md

+
+[RFC 1969]: https://github.com/rust-lang/rfcs/pull/1969
+[replace]: specifying-dependencies.html#overriding-dependencies
+


Regarding docs, we have an overwhelming number of options to override a package from crates.io:

augment

replace

path overrides

path dependencies

vendoring

Perhaps we could create a new section "using non crates io depedendencies", which gives a flow chart style guide of choosing a right tool for the problem?

Sounds plausible to me! I plan to focus on these docs once we land the RFC

matklad · 2017-06-07T21:33:29Z

tests/cargotest/support/mod.rs

        self
    }

+    fn _with_stderr(&mut self, expected: &ToString) {
+        self.expect_stderr = Some(expected.to_string());
+    }


I am curious, is this to improve compile times? Does it really help measurably?

It started off as that yeah, I was astounded at how long it took a "hello world" cargo test to compile so was randomly trying things to push down compile times, none of it ever ended up working though :(

bors · 2017-07-11T19:49:53Z

☔ The latest upstream changes (presumably #4259) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2017-07-12T15:16:40Z

I've updated with documentation and a renaming to [patch], r? @matklad

SimonSapin · 2017-07-18T21:45:58Z

Alright, building Cargo locally for testing brand-new features sounds reasonable (at least in terms of compile times compared to building rustc).

I’m not sure how to use it, though. Can I just copy target/release/cargo around, or does "installing" Cargo also involves copying some other files?

Update to cargo 0.21.0-nightly (eb6cf012a 2017-07-02) Cargo binaries are now produced on Rust’s CI, not Cargo’s. Remove cargo-commit-hash and find Cargo based on rust-commit-hash. Unfortunately, Cargo binaries are not available for every Cargo PR anymore: rust-lang/cargo#4123 (comment)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17780)

matklad · 2017-07-18T21:49:16Z

Copying cargo binary should be fine. I usually do export CARGO_DEV=~/projects/cargo/target/dev/cargo and use $CARGO_DEV instead of cargo then.

SimonSapin · 2017-07-18T21:51:32Z

@matklad When building Servo you don’t typically run Cargo directly, so it’s a little more involved. But I’ll figure something out.

matklad · 2017-07-18T21:54:25Z

Hm, I wonder if cargo install --git https://github.com/rust-lang/cargo --root ~/bin could help :)

I think you can even cargo install --git https://github.com/rust-lang/cargo --force =P

SimonSapin · 2017-07-18T21:57:13Z

Interesting. But I think I’d rather have cargo be managed by rustup usually, and only when I want to test something specific compile from git.

Update to cargo 0.21.0-nightly (eb6cf012a 2017-07-02) Cargo binaries are now produced on Rust’s CI, not Cargo’s. Remove cargo-commit-hash and find Cargo based on rust-commit-hash. Unfortunately, Cargo binaries are not available for every Cargo PR anymore: rust-lang/cargo#4123 (comment)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17780)

…07-02) (from servo:cargoup); r=nox,SimonSapin Cargo binaries are now produced on Rust’s CI, not Cargo’s. Remove cargo-commit-hash and find Cargo based on rust-commit-hash. Unfortunately, Cargo binaries are not available for every Cargo PR anymore: rust-lang/cargo#4123 (comment) Source-Repo: https://github.com/servo/servo Source-Revision: d403f404382c66485d9744787ce021556be59d6c --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 5adb79e9d2a0446949f4ca8211496d96b41e648f

…07-02) (from servo:cargoup); r=nox,SimonSapin Cargo binaries are now produced on Rust’s CI, not Cargo’s. Remove cargo-commit-hash and find Cargo based on rust-commit-hash. Unfortunately, Cargo binaries are not available for every Cargo PR anymore: rust-lang/cargo#4123 (comment) Source-Repo: https://github.com/servo/servo Source-Revision: d403f404382c66485d9744787ce021556be59d6c

rust-lang/cargo#4123 added the [patch] section of the manifest

Update Cargo to ffab51954ec32d55631c37a8730bb24915fc090b rust-lang/cargo#4123 added the `[patch]` section of the manifest

…07-02) (from servo:cargoup); r=nox,SimonSapin Cargo binaries are now produced on Rust’s CI, not Cargo’s. Remove cargo-commit-hash and find Cargo based on rust-commit-hash. Unfortunately, Cargo binaries are not available for every Cargo PR anymore: rust-lang/cargo#4123 (comment) Source-Repo: https://github.com/servo/servo Source-Revision: d403f404382c66485d9744787ce021556be59d6c

Changelog: Version 1.21.0 (2017-10-12) ========================== Language -------- - [You can now use static references for literals.][43838] Example: ```rust fn main() { let x: &'static u32 = &0; } ``` - [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540] Example: ```rust my_macro!(Vec<i32>::new); // Always worked my_macro!(Vec::<i32>::new); // Now works ``` Compiler -------- - [Upgraded jemalloc to 4.5.0][43911] - [Enabled unwinding panics on Redox][43917] - [Now runs LLVM in parallel during translation phase.][43506] This should reduce peak memory usage. Libraries --------- - [Generate builtin impls for `Clone` for all arrays and tuples that are `T: Clone`][43690] - [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459] - [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`, `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565] Stabilized APIs --------------- [`std::mem::discriminant`] Cargo ----- - [You can now call `cargo install` with multiple package names][cargo/4216] - [Cargo commands inside a virtual workspace will now implicitly pass `--all`][cargo/4335] - [Added a `[patch]` section to `Cargo.toml` to handle prepublication dependencies][cargo/4123] [RFC 1969] - [`include` & `exclude` fields in `Cargo.toml` now accept gitignore like patterns][cargo/4270] - [Added the `--all-targets` option][cargo/4400] - [Using required dependencies as a feature is now deprecated and emits a warning][cargo/4364] Misc ---- - [Cargo docs are moving][43916] to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo) - [The rustdoc book is now available][43863] at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc) - [Added a preview of RLS has been made available through rustup][44204] Install with `rustup component add rls-preview` - [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348] Previously only showed `std::os::unix`. Compatibility Notes ------------------- - [Changes in method matching against higher-ranked types][43880] This may cause breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880] - [rustc's JSON error output's byte position start at top of file.][42973] Was previously relative to the rustc's internal `CodeMap` struct which required the unstable library `libsyntax` to correctly use. - [`unused_results` lint no longer ignores booleans][43728] [42565]: rust-lang/rust#42565 [42973]: rust-lang/rust#42973 [43348]: rust-lang/rust#43348 [43459]: rust-lang/rust#43459 [43506]: rust-lang/rust#43506 [43540]: rust-lang/rust#43540 [43690]: rust-lang/rust#43690 [43728]: rust-lang/rust#43728 [43838]: rust-lang/rust#43838 [43863]: rust-lang/rust#43863 [43880]: rust-lang/rust#43880 [43911]: rust-lang/rust#43911 [43916]: rust-lang/rust#43916 [43917]: rust-lang/rust#43917 [44204]: rust-lang/rust#44204 [cargo/4123]: rust-lang/cargo#4123 [cargo/4216]: rust-lang/cargo#4216 [cargo/4270]: rust-lang/cargo#4270 [cargo/4335]: rust-lang/cargo#4335 [cargo/4364]: rust-lang/cargo#4364 [cargo/4400]: rust-lang/cargo#4400 [RFC 1969]: rust-lang/rfcs#1969 [info/43880]: rust-lang/rust#44224 (comment) [`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html

…07-02) (from servo:cargoup); r=nox,SimonSapin Cargo binaries are now produced on Rust’s CI, not Cargo’s. Remove cargo-commit-hash and find Cargo based on rust-commit-hash. Unfortunately, Cargo binaries are not available for every Cargo PR anymore: rust-lang/cargo#4123 (comment) Source-Repo: https://github.com/servo/servo Source-Revision: d403f404382c66485d9744787ce021556be59d6c UltraBlame original commit: 380ad1bfb187a075e540ffaf07e2c104d6c00a13

rust-highfive assigned brson Jun 5, 2017

alexcrichton requested a review from matklad June 5, 2017 14:34

rust-highfive assigned matklad and unassigned brson Jun 5, 2017

alexcrichton force-pushed the augment branch from e6b9b80 to d88c074 Compare June 5, 2017 16:55

matklad reviewed Jun 5, 2017

View reviewed changes

alexcrichton force-pushed the augment branch from acdcf6d to 9f67992 Compare June 6, 2017 14:51

matklad reviewed Jun 7, 2017

View reviewed changes

alexcrichton force-pushed the augment branch from 9f67992 to 1a991a3 Compare June 15, 2017 19:28

alexcrichton force-pushed the augment branch from 1a991a3 to 3493364 Compare July 12, 2017 15:12

alexcrichton changed the title ~~Implement the [augment] section of the manifest~~ Implement the [patch] section of the manifest Jul 12, 2017

alexcrichton closed this Jul 12, 2017

alexcrichton reopened this Jul 12, 2017

SimonSapin added a commit to SimonSapin/rust that referenced this pull request Jul 21, 2017

Update Cargo to ffab51954ec32d55631c37a8730bb24915fc090b

b77ff24

rust-lang/cargo#4123 added the [patch] section of the manifest

TimNN added a commit to TimNN/rust that referenced this pull request Jul 21, 2017

Rollup merge of rust-lang#43319 - SimonSapin:cargoup, r=alexcrichton

e345b9b

Update Cargo to ffab51954ec32d55631c37a8730bb24915fc090b rust-lang/cargo#4123 added the `[patch]` section of the manifest

bors added a commit to rust-lang/rust that referenced this pull request Jul 21, 2017

Auto merge of #43319 - SimonSapin:cargoup, r=alexcrichton

15aa15b

Update Cargo to ffab51954ec32d55631c37a8730bb24915fc090b rust-lang/cargo#4123 added the `[patch]` section of the manifest

SimonSapin mentioned this pull request Aug 25, 2017

Relax requirement that [replace] overrides have the same version number #2649

Closed

Eh2406 mentioned this pull request Jul 27, 2019

Refactor resolve Method #7186

Merged

ehuss added this to the 1.21.0 milestone Feb 6, 2022


		[RFC 1969]: https://github.com/rust-lang/rfcs/pull/1969
		[replace]: specifying-dependencies.html#overriding-dependencies

Implement the [patch] section of the manifest #4123

Implement the [patch] section of the manifest #4123

Conversation

alexcrichton commented Jun 5, 2017 • edited Loading

rust-highfive commented Jun 5, 2017

alexcrichton commented Jun 5, 2017

alexcrichton commented Jun 5, 2017

bors commented Jun 5, 2017

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 11, 2017

alexcrichton commented Jul 12, 2017

SimonSapin commented Jul 18, 2017

matklad commented Jul 18, 2017 • edited Loading

SimonSapin commented Jul 18, 2017

matklad commented Jul 18, 2017

SimonSapin commented Jul 18, 2017

alexcrichton commented Jun 5, 2017 •

edited

Loading

matklad commented Jul 18, 2017 •

edited

Loading