New command: cargo-upgrade #138

bjgill · 2017-07-05T18:18:01Z

This adds cargo upgrade as a more fully featured replacement for cargo add --update-only.

Usage:

Upgrade all dependencies in a manifest file to the latest version.

Usage:
    cargo upgrade [--dependency <dep>...] [--manifest-path <path>]
    cargo upgrade (-h | --help)
    cargo upgrade (-V | --version)

Options:
    -d --dependency <dep>       Specific dependency to upgrade. If this option is used, only the
                                specified dependencies will be upgraded.
    --manifest-path <path>      Path to the manifest to upgrade.
    -h --help                   Show this help page.
    -V --version                Show version.

Dev, build, and all target dependencies will also be upgraded. Only dependencies from crates.io are
supported. Git/path dependencies will be ignored.

Resolves #74.

Next steps (beyond this PR; probably not in v0.2) will be:

Workspace support
interactive 'check mode'
Use cargo to parse the manifests

(I'll squash prior to merging)

bjgill · 2017-07-05T18:30:07Z

r? @killercup @ordian

ordian · 2017-07-05T19:28:08Z

src/bin/upgrade/main.rs

+
+Options:
+    -h, --help                   Print this message
+    --dependency -d <dep>        Dependency to update


nit: can it be -d, --dependency <dep> for consistency in usage?
Also, -V, --version.
Same in README.

Also it's worth mentioning here that upgrade will update dev, build and all target dependencies as well.

OK. Though -d --dependency <dep> (etc.) is probably more consistent with cargo add/rm,

Right, lets use -d --dependency <dep> instead.

ordian · 2017-07-05T19:46:28Z

Looks good overall.

Do you think we should somehow take into account overriding dependencies (I think we shouldn't)?

ordian · 2017-07-05T20:06:20Z

README.md

+# Upgrade all dependencies
+$ cargo update
+# Upgrade libc and serde
+$ cargo update -d libc --dependency serde


Should this be cargo upgrade -d libc serde instead?
Also typo: update -> upgrade.

I don't think so - the usage syntax is that you need one of -d/--dependency per dependency to be upgraded. This follows the pattern that cargo update uses.

ordian · 2017-07-05T20:10:33Z

tests/test_manifest.rs

+                  "foo",
+                  "--manifest-path=tests/fixtures/manifest-invalid/Cargo.toml.sample"] =>
+                Error 1,
+"Command failed due to unhandled error: failed to parse datetime for key `invalid-section.key`")


nit: indentation here seems a bit off.

ordian · 2017-07-05T20:16:27Z

src/bin/upgrade/main.rs

+    let manifest_path = manifest_path.as_ref().map(From::from);
+    let mut manifest = Manifest::open(&manifest_path).unwrap();
+
+    // Look for dependencies in all three sections.


TIL: target dev-dependencies.

ordian · 2017-07-05T22:26:13Z

src/manifest.rs

+            })
+            .collect::<Vec<_>>();
+
+        // ... or in `target.<target name>.(dev-)dependencies`.


Looks like target build-dependencies is also a thing.

ordian · 2017-07-05T23:16:17Z

@bjgill Thank you!
💯

bjgill · 2017-07-06T18:18:17Z

@killercup - do you want to give this a look? Otherwise, I'll merge on Monday

killercup · 2017-07-06T21:50:55Z

Thanks for the ping! I'll try to review it this weekend. I originally also planned to release a 0.2 on the weekend/early next week, so it'd be nice to have this in. Benjamin Gill <notifications@github.com> schrieb am Do., 6. Juli 2017 um 20:18 Uhr:

…

@killercup <https://github.com/killercup> - do you want to give this a look? Otherwise, I'll merge on Monday — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX6rtmeU9FEfUR-e4gE5j735l6LXMks5sLSTpgaJpZM4OOsLP> .

bjgill · 2017-07-17T22:59:05Z

I've now added the change to switch cargo-upgrade to using cargo to parse manifest files. This adds a lot of new stuff to Cargo.lock but is otherwise mostly a partial re-write of cargo-upgrade, hence its inclusion in this PR.

Once merged, this will be straightforwardly extensible to add proper workspace support.

@killercup - were you wanting to review this?

killercup · 2017-07-18T19:30:56Z

Sorry, I'm sooo behind on my reviews.

Quick thought: Instead of using cargo as a library (that includes a truckload of dependencies I assume), we can also run cargo metadata and parse its JSON output.

bjgill · 2017-07-18T23:11:00Z

Yes. As the AppVeyor CI is showing, the non-Rust dependencies of cargo are quite a pain.

bjgill · 2017-07-18T23:21:28Z

In fact, I think the switch to cargo is probably not worth the hassle of getting it into this PR/v0.2.

I've reset to the commit prior to the introduction of cargo. This PR (and v0.2) doesn't need to be delayed any further by me faffing around with the back-end - it should be fine to go in as-is. We can wait to switch back-end and add workspace support in v0.2.1/v0.3.

EDIT: for future reference, the removed commits can be found in bjgill#2

killercup

Alright, I've finally read through this. Very nice! I left a few inline comments, but nothing major. Is there a test for updating a dependency only available in one target?

Feel free to merge this once you think it's good to go!

killercup · 2017-07-19T14:32:24Z

src/bin/upgrade/main.rs

+            // Not a version dependency if the `git` or `path` keys are present.
+            !(table.contains_key("git") || table.contains_key("path"))
+        })
+        .unwrap_or(true)


The comment seems redundant as the code says basically the same thing. I also think I'd nowadays write this as

if let Some(table) = dep.as_table() { !table.contains_key("git") && !table.contains_key("path") } else { true }

killercup · 2017-07-19T14:33:10Z

src/bin/upgrade/main.rs

+            })
+            .filter(|&(_name, old_value)| is_version_dependency(old_value))
+            .map(|(name, _old_value)| {
+


killercup · 2017-07-19T14:36:48Z

src/bin/upgrade/main.rs

+
+                Ok(())
+            })
+            .collect::<Result<Vec<_>, Box<Error>>>()?;


You are collecting this into a Vec… just to drop it? Seems weird. Better use an inner for-loop with the body of the map (this way the ? will return in the fn scope)

killercup · 2017-07-19T14:50:16Z

src/manifest.rs

+            if let Some(segment) = path.pop_front() {
+                let value = match *input
+                    .entry(segment.to_owned())
+                    .or_insert_with(|| toml::Value::Table(BTreeMap::new())) {


This is hard to read. Can you give the thing you are matching over its own binding and keep the match head single-line?

killercup · 2017-07-19T14:51:11Z

src/manifest.rs

+            }
+        }
+
+        descend(&mut self.data, table_path.into_iter().collect())


Nicely done.

ordian · 2017-07-19T15:25:39Z

src/manifest.rs

+        /// Descend into a manifest until the required table is found.
+        fn descend<'a>(
+            input: &'a mut BTreeMap<String, toml::Value>,
+            mut path: VecDeque<&String>,


Can we avoid allocation?
I think, if we make path: &[String], then
if let Some(segment) = path.pop_front() { can be replaced with if !path.is_empty() { let segment = path[0];
and descend(value, path) with descend(value, &path[1..])

Interesting, using sub slices is pretty clever :)

(But don't go out your way to optimize stuff like this; this whole project is in no way performance critical code!)

That is quite nice.

bjgill · 2017-07-19T23:11:00Z

@killercup and @ordian - thanks. The Appveyor failures are entirely spurious - it looks as if the revocation server they use is currently down. I'll squash and pass to bors to merge in.

This adds `cargo upgrade` as a more fully featured replacement for `cargo add --update-only`. Usage: ```plain Upgrade all dependencies in a manifest file to the latest version. Usage: cargo upgrade [--dependency <dep>...] [--manifest-path <path>] cargo upgrade (-h | --help) cargo upgrade (-V | --version) Options: -d --dependency <dep> Specific dependency to upgrade. If this option is used, only the specified dependencies will be upgraded. --manifest-path <path> Path to the manifest to upgrade. -h --help Show this help page. -V --version Show version. Dev, build, and all target dependencies will also be upgraded. Only dependencies from crates.io are supported. Git/path dependencies will be ignored. ``` Resolves killercup#74.

bjgill · 2017-07-19T23:23:41Z

bors r+

138: New command: cargo-upgrade r=bjgill This adds `cargo upgrade` as a more fully featured replacement for `cargo add --update-only`. Usage: ```plain Upgrade all dependencies in a manifest file to the latest version. Usage: cargo upgrade [--dependency <dep>...] [--manifest-path <path>] cargo upgrade (-h | --help) cargo upgrade (-V | --version) Options: -d --dependency <dep> Specific dependency to upgrade. If this option is used, only the specified dependencies will be upgraded. --manifest-path <path> Path to the manifest to upgrade. -h --help Show this help page. -V --version Show version. Dev, build, and all target dependencies will also be upgraded. Only dependencies from crates.io are supported. Git/path dependencies will be ignored. ``` Resolves #74. Next steps (beyond this PR; probably not in v0.2) will be: * Workspace support * interactive 'check mode' * Use `cargo` to parse the manifests (I'll squash prior to merging)

bors · 2017-07-19T23:25:40Z

Build failed

continuous-integration/appveyor/branch

killercup · 2017-07-19T23:41:44Z

Yeah, for most rust projects appveyor seems quiet slow and fragile… feel free to remove it from bors' config. bors[bot] <notifications@github.com> schrieb am Do. 20. Juli 2017 um 01:25:

…

Build failed - continuous-integration/appveyor/branch <https://ci.appveyor.com/project/killercup/cargo-edit/build/1.0.26> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX9gkeT2obh0XeGdOjInNGWoupSPzks5sPpB1gaJpZM4OOsLP> .

It's currently too unreliable

bjgill · 2017-07-19T23:52:31Z

Ok. I've raised #139 to cover re-instating Appveyor in bors

bors r+

138: New command: cargo-upgrade r=bjgill This adds `cargo upgrade` as a more fully featured replacement for `cargo add --update-only`. Usage: ```plain Upgrade all dependencies in a manifest file to the latest version. Usage: cargo upgrade [--dependency <dep>...] [--manifest-path <path>] cargo upgrade (-h | --help) cargo upgrade (-V | --version) Options: -d --dependency <dep> Specific dependency to upgrade. If this option is used, only the specified dependencies will be upgraded. --manifest-path <path> Path to the manifest to upgrade. -h --help Show this help page. -V --version Show version. Dev, build, and all target dependencies will also be upgraded. Only dependencies from crates.io are supported. Git/path dependencies will be ignored. ``` Resolves #74. Next steps (beyond this PR; probably not in v0.2) will be: * Workspace support * interactive 'check mode' * Use `cargo` to parse the manifests (I'll squash prior to merging)

bors · 2017-07-19T23:59:21Z

Build succeeded

continuous-integration/travis-ci/push

bjgill changed the title ~~Cargo upgrade~~ New command: cargo-upgrade Jul 5, 2017

ordian requested changes Jul 5, 2017

View reviewed changes

ordian reviewed Jul 5, 2017

View reviewed changes

bjgill mentioned this pull request Jul 17, 2017

Use cargo to parse manifest bjgill/cargo-edit#2

Merged

bjgill force-pushed the cargo-upgrade branch from f0708f8 to e3a16e3 Compare July 18, 2017 23:14

killercup approved these changes Jul 19, 2017

View reviewed changes

ordian reviewed Jul 19, 2017

View reviewed changes

bjgill force-pushed the cargo-upgrade branch from ad2da1a to 08f07d1 Compare July 19, 2017 23:17

Remove Appveyor from bors

c0c4067

It's currently too unreliable

bors bot merged commit c0c4067 into killercup:master Jul 19, 2017

bjgill deleted the cargo-upgrade branch July 20, 2017 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New command: cargo-upgrade #138

New command: cargo-upgrade #138

bjgill commented Jul 5, 2017 •

edited

bjgill commented Jul 5, 2017

ordian Jul 5, 2017

ordian Jul 5, 2017

bjgill Jul 5, 2017

ordian Jul 5, 2017

ordian commented Jul 5, 2017

ordian Jul 5, 2017

bjgill Jul 5, 2017

ordian Jul 5, 2017

ordian Jul 5, 2017

ordian Jul 5, 2017

ordian commented Jul 5, 2017

bjgill commented Jul 6, 2017

killercup commented Jul 6, 2017 via email

bjgill commented Jul 17, 2017

killercup commented Jul 18, 2017

bjgill commented Jul 18, 2017

bjgill commented Jul 18, 2017 •

edited

killercup left a comment

killercup Jul 19, 2017

killercup Jul 19, 2017

killercup Jul 19, 2017

killercup Jul 19, 2017

killercup Jul 19, 2017

ordian Jul 19, 2017

killercup Jul 19, 2017

bjgill Jul 19, 2017

bjgill commented Jul 19, 2017

bjgill commented Jul 19, 2017

bors bot commented Jul 19, 2017

killercup commented Jul 19, 2017 via email

bjgill commented Jul 19, 2017

bors bot commented Jul 19, 2017

New command: cargo-upgrade #138

New command: cargo-upgrade #138

Conversation

bjgill commented Jul 5, 2017 • edited

bjgill commented Jul 5, 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

ordian commented Jul 5, 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

ordian commented Jul 5, 2017

bjgill commented Jul 6, 2017

killercup commented Jul 6, 2017 via email

bjgill commented Jul 17, 2017

killercup commented Jul 18, 2017

bjgill commented Jul 18, 2017

bjgill commented Jul 18, 2017 • edited

killercup left a 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

bjgill commented Jul 19, 2017

bjgill commented Jul 19, 2017

bors bot commented Jul 19, 2017

Build failed

killercup commented Jul 19, 2017 via email

bjgill commented Jul 19, 2017

bors bot commented Jul 19, 2017

Build succeeded

bjgill commented Jul 5, 2017 •

edited

bjgill commented Jul 18, 2017 •

edited