Skip to content

Conversation

nrc
Copy link
Member

@nrc nrc commented Sep 13, 2017

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good! A few thoughts:

  • Do you think we should wait for #43628 to have a TOML file to enable/disable the tool?
  • Do you want to go ahead and create a rustfmt-preview component for rustup? Or leave that to a future PR?

.gitmodules Outdated
@@ -36,3 +36,6 @@
[submodule "src/tools/clippy"]
path = src/tools/clippy
url = https://github.com/rust-lang-nursery/rust-clippy.git
[submodule "src/tools/rustfmt"]
path = src/tools/rustfmt
url = [email protected]:rust-lang-nursery/rustfmt.git
Copy link
Member

Choose a reason for hiding this comment

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

Could this use an https url like the other submodules? I don't know why the others don't use ssh, bug may as well be consistent I guess?

@@ -58,3 +59,9 @@ debug-assertions = false

[patch.'https://github.com/rust-lang/cargo']
cargo = { path = "tools/cargo" }

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here for why there's two sections?

@nrc
Copy link
Member Author

nrc commented Sep 13, 2017

Do you think we should wait for #43628 to have a TOML file to enable/disable the tool?

We're already building with the RLS, so it doesn't feel like we need to wait for that (I can't imagine a change to the compiler causing a Rustfmt test to fail, just stop it building), and landing this will make it easier to land changes which are breaking.

Do you want to go ahead and create a rustfmt-preview component for rustup? Or leave that to a future PR?

I was going to do this later, there are probably a few things we should do in rustfmt before it should be rustup-installable.

Will address the inline points...

@alexcrichton
Copy link
Member

Ok sounds good to me. Can you verify that the x86_64-gnu-aux builder runs rustfmt tests? I think you'll need to update src/bootstrap/mk/Makefile.in as well to get it to run those.

@nrc
Copy link
Member Author

nrc commented Sep 13, 2017

Added the change to Makefile.in. To verify, can I just check the Travis logs?

@alexcrichton
Copy link
Member

Nah travis won't run the builder by default, but you can temporarily add ALLOW_PR=1 to the right entry in .travis.yml to allow it on this PR

@nrc
Copy link
Member Author

nrc commented Sep 13, 2017

And now we wait...

@nrc
Copy link
Member Author

nrc commented Sep 13, 2017

OK, builder is running test:

``` [01:40:56] travis_fold:start:stage2-rustfmt �[0Ktravis_time:start:stage2-rustfmt �[0KBuilding stage2 tool rustfmt (x86_64-unknown-linux-gnu) [01:40:57] �[m�[m�[32m�[1m Compiling�[m rustfmt-nightly v0.2.5 (file:///checkout/src/tools/rustfmt) [01:41:05] �[m�[m�[32m�[1m Finished�[m release [optimized] target(s) in 8.58 secs [01:41:05] travis_fold:end:stage2-rustfmt �[0K [01:41:05] travis_time:end:stage2-rustfmt:start=1505284842232889311,finish=1505284851091334990,duration=8858445679 �[0K [01:41:06] �[m�[m�[32m�[1m Compiling�[m rustfmt-nightly v0.2.5 (file:///checkout/src/tools/rustfmt) [01:41:42] �[m�[m�[32m�[1m Finished�[m release [optimized] target(s) in 36.45 secs [01:41:42] �[m�[m�[32m�[1m Running�[m build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/rustfmt_nightly-5ec473560ad2fd8b [01:41:42] [01:41:42] running 27 tests [01:41:42] test comment::test::char_classes ... ok [01:41:42] test comment::test::comment_code_slices_three ... ok [01:41:42] test comment::test::comment_code_slices_two ... ok [01:41:42] test comment::test::comment_code_slices ... ok [01:41:42] test comment::test::format_comments ... ok [01:41:42] test comment::test::test_contains_comment ... ok [01:41:42] test comment::test::test_find_uncommented ... ok [01:41:42] test comment::test::test_uncommented ... ok [01:41:42] test config::test::test_config_set ... ok [01:41:42] test config::test::test_config_used_to_toml ... ok [01:41:42] test config::test::test_was_set ... ok [01:41:42] test file_lines::test::test_range_adjacent_to ... ok [01:41:42] test file_lines::test::test_range_contains ... ok [01:41:42] test file_lines::test::test_range_intersects ... ok [01:41:42] test file_lines::test::test_range_merge ... ok [01:41:42] test issues::find_issue ... ok [01:41:42] test issues::find_unnumbered_issue ... ok [01:41:42] test issues::issue_type ... ok [01:41:42] test test::indent_add_sub ... ok [01:41:42] test string::test::issue343 ... ok [01:41:42] test test::indent_to_string_spaces ... ok [01:41:42] test test::indent_to_string_hard_tabs ... ok [01:41:42] test test::indent_add_sub_alignment ... ok [01:41:42] test test::shape_block_indent_with_alignment ... ok [01:41:42] test test::shape_block_indent_without_alignment ... ok [01:41:42] test test::shape_visual_indent ... ok [01:41:42] test utils::bin_search_test ... ok [01:41:42] [01:41:42] test result: ok. 27 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out [01:41:42] [01:41:42] �[m�[m�[32m�[1m Running�[m build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/cargo_fmt-836cde323a42a549 [01:41:42] [01:41:42] running 0 tests [01:41:42] [01:41:42] test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out [01:41:42] [01:41:42] �[m�[m�[32m�[1m Running�[m build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/rustfmt-8c463cbf88ea85ab [01:41:42] [01:41:42] running 0 tests [01:41:42] [01:41:42] test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out [01:41:42] [01:41:42] �[m�[m�[32m�[1m Running�[m build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/rustfmt_format_diff-06a931d4d37505c4 [01:41:42] [01:41:42] running 1 test [01:41:42] test scan_simple_git_diff ... ok [01:41:42] [01:41:42] test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out [01:41:42] [01:41:42] �[m�[m�[32m�[1m Running�[m build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/system-b8aa368703c9e712 [01:41:42] [01:41:42] running 9 tests [01:41:42] test format_lines_errors_are_reported ... ok [01:41:42] test coverage_tests ... ok [01:41:42] test checkstyle_test ... ok [01:41:42] test rustfmt_diff_no_diff_test ... ok [01:41:42] test rustfmt_diff_make_diff_tests ... ok [01:41:42] test stdin_formatting_smoke_test ... ok [01:41:42] test system_tests ... ok [01:41:42] test idempotence_tests ... ok [01:41:42] test self_tests ... ok [01:41:42] [01:41:42] test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out ```

@nrc
Copy link
Member Author

nrc commented Sep 13, 2017

And pushed PR without the travis.yaml change.

@alexcrichton r?

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Sep 13, 2017

📌 Commit 368cab3 has been approved by alexcrichton

@aidanhs aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 13, 2017
# Override rustfmt dependencies both on the repo and the crate (the RLS
# sometimes uses either).
# FIXME should only need the crates.io patch, long term.
[patch.'https://github.com/rust-lang-nursery/rustfmt']
Copy link
Contributor

Choose a reason for hiding this comment

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

single quotes confused some editors (e.g. the vscode toml plugin) - could you make them double quotes?

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
bors added a commit that referenced this pull request Sep 17, 2017
Rollup of 17 pull requests

- Successful merges: #44073, #44088, #44381, #44397, #44509, #44533, #44549, #44553, #44562, #44567, #44595, #44604, #44617, #44622, #44630, #44639, #44647
- Failed merges:
@bors bors merged commit 368cab3 into rust-lang:master Sep 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants