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

Import `cargo fix` directly in to Cargo #5723

Merged
merged 1 commit into from Jul 17, 2018

Conversation

Projects
None yet
8 participants
@alexcrichton
Member

alexcrichton commented Jul 14, 2018

This commit imports the cargo fix subcommand in rust-lang-nursery/rustfix
directly into Cargo as a subcommand. This should allow us to ease our
distribution story of cargo fix as we prepare for the upcoming 2018 edition
release.

It's been attempted here to make the code as idiomatic as possible for Cargo's
own codebase. Additionally all tests from cargo-fix were imported into Cargo's
test suite as well. After this lands and is published in nightly the cargo-fix
command in rust-lang-nursery/rustfix will likely be removed.

cc rust-lang/rust#52272

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 14, 2018

r? @matklad

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

rust-highfive commented Jul 14, 2018

r? @matklad

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented Jul 14, 2018

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Jul 14, 2018

Member

Cool! Thanks for doing this, @alexcrichton!

I probably would've tried to carry over the commit messages using git-subtree but other than that it looks like you managed to make this fit quite well into cargo. Not sure how idiomatic this is in cargo, but maybe you can also split up the tests further? 1k lines is quite a lot to scroll through without grouping :)

(Also: Remind me to add support for message-format=json to cargo-fix.)

CI failures:

  • Mac/Windows: Git config?

    'repo.signature() failed with config value 'user.name' was not found; class=Config (7); code=NotFound (-3)'

  • Windows: Issues with placeholders and path separators in tests:
    1 - |[FIXING] bar/src/lib.rs (1 fix)|
      + |      Fixing bar\src\lib.rs (1 fix)|
    
Member

killercup commented Jul 14, 2018

Cool! Thanks for doing this, @alexcrichton!

I probably would've tried to carry over the commit messages using git-subtree but other than that it looks like you managed to make this fit quite well into cargo. Not sure how idiomatic this is in cargo, but maybe you can also split up the tests further? 1k lines is quite a lot to scroll through without grouping :)

(Also: Remind me to add support for message-format=json to cargo-fix.)

CI failures:

  • Mac/Windows: Git config?

    'repo.signature() failed with config value 'user.name' was not found; class=Config (7); code=NotFound (-3)'

  • Windows: Issues with placeholders and path separators in tests:
    1 - |[FIXING] bar/src/lib.rs (1 fix)|
      + |      Fixing bar\src\lib.rs (1 fix)|
    
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 15, 2018

Member

Ah we've got pretty long tests in Cargo unfortunately, most of it's due to unnecessarily verbose test cases. We should definitely fix that at some point!

CI should be green now too!

Member

alexcrichton commented Jul 15, 2018

Ah we've got pretty long tests in Cargo unfortunately, most of it's due to unnecessarily verbose test cases. We should definitely fix that at some point!

CI should be green now too!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 16, 2018

Member

r? @ehuss

@bors: delegate=ehuss

Member

alexcrichton commented Jul 16, 2018

r? @ehuss

@bors: delegate=ehuss

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 16, 2018

Contributor

✌️ @ehuss can now approve this pull request

Contributor

bors commented Jul 16, 2018

✌️ @ehuss can now approve this pull request

@killercup

lgtm

// not the best heuristic but matches what Cargo does today at least.
let mut fixes = FixedCrate::default();
if let Some(path) = filename {
if !Path::new(&path).is_absolute() {

This comment has been minimized.

@ehuss

ehuss Jul 16, 2018

Contributor

This also sweeps up path dependencies. That might be surprising.

@ehuss

ehuss Jul 16, 2018

Contributor

This also sweeps up path dependencies. That might be surprising.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 17, 2018

Member

Correct! That's actually intentional though in the sense that if it's a path dependency you're probably working on it so we go ahead and fix it.

@alexcrichton

alexcrichton Jul 17, 2018

Member

Correct! That's actually intentional though in the sense that if it's a path dependency you're probably working on it so we go ahead and fix it.

Show outdated Hide outdated src/cargo/ops/fix.rs
Show outdated Hide outdated tests/testsuite/cargotest/support/mod.rs
@ehuss

This comment has been minimized.

Show comment
Hide comment
@ehuss

ehuss Jul 16, 2018

Contributor

cargo fix does not play well with running cargo check. For example, if your editor automatically runs cargo check, then cargo fix will do nothing because it thinks everything is "fresh". I'm inclined to just file an issue for this and see if it can be fixed after this merges?

Contributor

ehuss commented Jul 16, 2018

cargo fix does not play well with running cargo check. For example, if your editor automatically runs cargo check, then cargo fix will do nothing because it thinks everything is "fresh". I'm inclined to just file an issue for this and see if it can be fixed after this merges?

Import `cargo fix` directly in to Cargo
This commit imports the `cargo fix` subcommand in rust-lang-nursery/rustfix
directly into Cargo as a subcommand. This should allow us to ease our
distribution story of `cargo fix` as we prepare for the upcoming 2018 edition
release.

It's been attempted here to make the code as idiomatic as possible for Cargo's
own codebase. Additionally all tests from cargo-fix were imported into Cargo's
test suite as well. After this lands and is published in nightly the `cargo-fix`
command in rust-lang-nursery/rustfix will likely be removed.

cc rust-lang/rust#52272
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 17, 2018

Member

I'm inclined to just file an issue for this and see if it can be fixed after this merges?

Ah yeah there's a few issues on rust-lang-nursery/rustfix like this one which we should transfer over once merged

Member

alexcrichton commented Jul 17, 2018

I'm inclined to just file an issue for this and see if it can be fixed after this merges?

Ah yeah there's a few issues on rust-lang-nursery/rustfix like this one which we should transfer over once merged

@ehuss

This comment has been minimized.

Show comment
Hide comment
@ehuss

ehuss Jul 17, 2018

Contributor

@bors r+

Contributor

ehuss commented Jul 17, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 17, 2018

Contributor

📌 Commit b02ba37 has been approved by ehuss

Contributor

bors commented Jul 17, 2018

📌 Commit b02ba37 has been approved by ehuss

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 17, 2018

Contributor

⌛️ Testing commit b02ba37 with merge 506eea7...

Contributor

bors commented Jul 17, 2018

⌛️ Testing commit b02ba37 with merge 506eea7...

bors added a commit that referenced this pull request Jul 17, 2018

Auto merge of #5723 - alexcrichton:cargo-fix, r=ehuss
Import `cargo fix` directly in to Cargo

This commit imports the `cargo fix` subcommand in rust-lang-nursery/rustfix
directly into Cargo as a subcommand. This should allow us to ease our
distribution story of `cargo fix` as we prepare for the upcoming 2018 edition
release.

It's been attempted here to make the code as idiomatic as possible for Cargo's
own codebase. Additionally all tests from cargo-fix were imported into Cargo's
test suite as well. After this lands and is published in nightly the `cargo-fix`
command in rust-lang-nursery/rustfix will likely be removed.

cc rust-lang/rust#52272
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 17, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: ehuss
Pushing 506eea7 to master...

Contributor

bors commented Jul 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: ehuss
Pushing 506eea7 to master...

@bors bors merged commit b02ba37 into rust-lang:master Jul 17, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Jul 17, 2018

Member

🎉

Member

killercup commented Jul 17, 2018

🎉

@alexcrichton alexcrichton deleted the alexcrichton:cargo-fix branch Jul 17, 2018

alexcrichton added a commit to alexcrichton/rustfix that referenced this pull request Jul 17, 2018

Delete `cargo fix`
This has now moved upstream into Cargo itself at rust-lang/cargo#5723

@alexcrichton alexcrichton referenced this pull request Jul 17, 2018

Merged

Delete `cargo fix` #132

@killercup killercup referenced this pull request Jul 18, 2018

Open

Tools news #37

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jul 18, 2018

Member

Ah we've got pretty long tests in Cargo unfortunately, most of it's due to unnecessarily verbose test cases. We should definitely fix that at some point!

That sounds like a nice E-easy piece of grunt work. If you were to flesh out your thoughts on how to do that I might be able to help. 😄

Member

dwijnand commented Jul 18, 2018

Ah we've got pretty long tests in Cargo unfortunately, most of it's due to unnecessarily verbose test cases. We should definitely fix that at some point!

That sounds like a nice E-easy piece of grunt work. If you were to flesh out your thoughts on how to do that I might be able to help. 😄

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 18, 2018

Member

@dwijnand an excellent point! I've filled out the A-testing-cargo-itself tag on our issue tracker a bit, and anything tagged with E-help-wanted would be most appreciated to have some help to tackle!

Member

alexcrichton commented Jul 18, 2018

@dwijnand an excellent point! I've filled out the A-testing-cargo-itself tag on our issue tracker a bit, and anything tagged with E-help-wanted would be most appreciated to have some help to tackle!

@MoSal

This comment has been minimized.

Show comment
Hide comment
@MoSal

MoSal Jul 20, 2018

I was wondering why a rustfix component wasn't added to rustup with clippy before finding this PR.

I think an announcement about this change should be made when the next nightly becomes available.

MoSal commented Jul 20, 2018

I was wondering why a rustfix component wasn't added to rustup with clippy before finding this PR.

I think an announcement about this change should be made when the next nightly becomes available.

@killercup killercup referenced this pull request Aug 1, 2018

Closed

August 2018 event #59

10 of 10 tasks complete

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 14, 2018

rust: Update to 1.29.0.
Version 1.29.0 (2018-09-13)
==========================

Compiler
--------
- [Bumped minimum LLVM version to 5.0.][51899]
- [Added `powerpc64le-unknown-linux-musl` target.][51619]
- [Added `aarch64-unknown-hermit` and `x86_64-unknown-hermit` targets.][52861]

Libraries
---------
- [`Once::call_once` now no longer requires `Once` to be `'static`.][52239]
- [`BuildHasherDefault` now implements `PartialEq` and `Eq`.][52402]
- [`Box<CStr>`, `Box<OsStr>`, and `Box<Path>` now implement `Clone`.][51912]
- [Implemented `PartialEq<&str>` for `OsString` and `PartialEq<OsString>`
  for `&str`.][51178]
- [`Cell<T>` now allows `T` to be unsized.][50494]
- [`SocketAddr` is now stable on Redox.][52656]

Stabilized APIs
---------------
- [`Arc::downcast`]
- [`Iterator::flatten`]
- [`Rc::downcast`]

Cargo
-----
- [Cargo can silently fix some bad lockfiles ][cargo/5831] You can use
  `--locked` to disable this behaviour.
- [`cargo-install` will now allow you to cross compile an install
  using `--target`][cargo/5614]
- [Added the `cargo-fix` subcommand to automatically move project code from
  2015 edition to 2018.][cargo/5723]

Misc
----
- [`rustdoc` now has the `--cap-lints` option which demotes all lints above
  the specified level to that level.][52354] For example `--cap-lints warn`
  will demote `deny` and `forbid` lints to `warn`.
- [`rustc` and `rustdoc` will now have the exit code of `1` if compilation
  fails, and `101` if there is a panic.][52197]
- [A preview of clippy has been made available through rustup.][51122]
  You can install the preview with `rustup component add clippy-preview`

Compatibility Notes
-------------------
- [`str::{slice_unchecked, slice_unchecked_mut}` are now deprecated.][51807]
  Use `str::get_unchecked(begin..end)` instead.
- [`std::env::home_dir` is now deprecated for its unintuitive behaviour.][51656]
  Consider using the `home_dir` function from
  https://crates.io/crates/dirs instead.
- [`rustc` will no longer silently ignore invalid data in target spec.][52330]

[52861]: rust-lang/rust#52861
[52656]: rust-lang/rust#52656
[52239]: rust-lang/rust#52239
[52330]: rust-lang/rust#52330
[52354]: rust-lang/rust#52354
[52402]: rust-lang/rust#52402
[52103]: rust-lang/rust#52103
[52197]: rust-lang/rust#52197
[51807]: rust-lang/rust#51807
[51899]: rust-lang/rust#51899
[51912]: rust-lang/rust#51912
[51511]: rust-lang/rust#51511
[51619]: rust-lang/rust#51619
[51656]: rust-lang/rust#51656
[51178]: rust-lang/rust#51178
[51122]: rust-lang/rust#51122
[50494]: rust-lang/rust#50494
[cargo/5614]: rust-lang/cargo#5614
[cargo/5723]: rust-lang/cargo#5723
[cargo/5831]: rust-lang/cargo#5831
[`Arc::downcast`]: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.downcast
[`Iterator::flatten`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flatten
[`Rc::downcast`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.downcast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment