Skip to content
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

Enforce rustfmt on CI #22126

Merged
merged 6 commits into from Nov 7, 2018
Merged

Enforce rustfmt on CI #22126

merged 6 commits into from Nov 7, 2018

Conversation

pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Nov 6, 2018

This change is Reviewable

@highfive
Copy link

highfive commented Nov 6, 2018

Heads up! This PR modifies the following files:

  • @jgraham: components/webdriver_server/lib.rs
  • @wafflespeanut: python/servo/devenv_commands.py, python/servo/testing_commands.py
  • @emilio: components/style/gecko_bindings/sugar/ns_css_value.rs, components/style/dom_apis.rs, components/style/gecko/url.rs, components/style/animation.rs, components/style/values/specified/text.rs and 58 more
  • @KiChjang: components/script/dom/request.rs, components/script/dom/timeranges.rs, components/script/dom/htmlcanvaselement.rs, components/script/textinput.rs, components/script/dom/eventtarget.rs and 80 more
  • @asajeffrey: components/script/dom/request.rs, components/script/dom/timeranges.rs, components/script/dom/htmlcanvaselement.rs, components/script/textinput.rs, components/script/dom/eventtarget.rs and 77 more
  • @cbrewster: components/constellation/network_listener.rs, components/constellation/constellation.rs, components/constellation/pipeline.rs
  • @paulrouget: components/compositing/build.rs, components/constellation/network_listener.rs, components/constellation/constellation.rs, components/servo/lib.rs, components/constellation/pipeline.rs and 1 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 6, 2018
@highfive
Copy link

highfive commented Nov 6, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@CYBAI
Copy link
Member

CYBAI commented Nov 6, 2018

If this one is merged, maybe we can close the RFC PR #20617 from @emilio and close the #21373 issue?

@pyfisch
Copy link
Contributor Author

pyfisch commented Nov 6, 2018

Yes this is a replacement for #20617. Rustfmt is now installed with rustup and not with cargo anymore.

@emilio
Copy link
Member

emilio commented Nov 6, 2018

If we do this, which makes more annoying for me to port Gecko changes, can we enable the reordering of imports and such, and remove the tidy one?

At least that'd be roughly neutral or even positive on the effort it takes me :)

Also, maybe, remove the match_block_trailing_comma thing while at it?

@@ -304,6 +304,12 @@ def test_content(self):
"tests/wpt/mozilla/.")
return 0

def install_rustfmt(self):
if self.call_rustup_run(["cargo", "fmt", "--version", "-q"],
stderr=open(os.devnull, "w")) != 0:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this leak the file descriptor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@jdm
Copy link
Member

jdm commented Nov 6, 2018

Let's have two more commits - one that disables the tidy check for import ordering, and one that enables rustfmt import reordering and formats the whole tree with it.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 6, 2018
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 6, 2018
@jdm
Copy link
Member

jdm commented Nov 6, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 5de5fab has been approved by jdm

@highfive highfive assigned jdm and unassigned Manishearth Nov 6, 2018
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 6, 2018
@jdm
Copy link
Member

jdm commented Nov 6, 2018

@bors-servo r-
test-tidy complains about some line lengths now.

@jdm jdm added S-fails-tidy `./mach test-tidy` reported errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 6, 2018
@jdm
Copy link
Member

jdm commented Nov 6, 2018

Also apparently travis is not set up to run cargo fmt yet:

./components/script/dom/bluetoothcharacteristicproperties.rs:6: Line is longer than 120 characters
./components/script/dom/bluetoothremotegattcharacteristic.rs:8: Line is longer than 120 characters
./components/script/dom/bluetoothremotegattcharacteristic.rs:10: Line is longer than 120 characters
./components/script/dom/bluetoothremotegattdescriptor.rs:8: Line is longer than 120 characters
Running the dependency licensing lint...
It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment
It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment
It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment

@jdm
Copy link
Member

jdm commented Nov 6, 2018

And the selftests failed:

======================================================================
FAIL: test_rust (servo_tidy_tests.test_tidy.CheckTidiness)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/servo/servo/python/tidy/servo_tidy_tests/test_tidy.py", line 108, in test_rust
    self.assertEqual('extra space after {', errors.next()[2])
AssertionError: 'extra space after {' != 'missing space before }'
----------------------------------------------------------------------
Ran 28 tests in 0.030s
FAILED (failures=1)
The command "./mach test-tidy --no-progress --self-test" exited with 1.

@pyfisch
Copy link
Contributor Author

pyfisch commented Nov 6, 2018

I've pushed the two commits as suggested by @jdm.

There are still a few problems with this PR:

  • On Travis CI the first build does not install rustup. For this reason ./mach test-tidy fails now. Install rustup there too? done

  • I used #![rustup::skip] in two files to stop rustfmt from mangling whitespace (introduce trailing whitespace to block comments) but this causes the error error[E0658]: non-builtin inner attributes are unstable. What to do here? use outer attributes
    *Rustfmt formats imports in 4 cases such that the lines are longer than 120 characters which causes a tidy error: use dom::bindings::codegen::Bindings::BluetoothCharacteristicPropertiesBinding::BluetoothCharacteristicPropertiesMethods; These cases are ignored with the is_unsplittable line test

  • Also, maybe, remove the match_block_trailing_comma thing while at it?

    I think this a good idea. It does not disrupt test-tidy.

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Nov 6, 2018
Add ./mach fmt command.
Mach installs rustfmt if needed.
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 6, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Nov 6, 2018

Rebased and fixed. Lets see if it works in Travis.

@jdm
Copy link
Member

jdm commented Nov 7, 2018

tidy reported no errors.
It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment
It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment
It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment
The command "./mach test-tidy --no-progress --all" exited with 1.

Install rustup on first Travis job.
Only use rustfmt::skip as an outer attribute.
@pyfisch
Copy link
Contributor Author

pyfisch commented Nov 7, 2018

Travis now passes.

AppVeyor has some connectivity issues:

The remote server returned an error: (503) Server Unavailable. Service Unavailable

@jdm
Copy link
Member

jdm commented Nov 7, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 1855c88 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Nov 7, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit 1855c88 with merge da2d9b2...

bors-servo pushed a commit that referenced this pull request Nov 7, 2018
Enforce rustfmt on CI

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22126)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, android-mac, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, status-taskcluster, windows-msvc-dev
Approved by: jdm
Pushing da2d9b2 to master...

@SimonSapin
Copy link
Member

Nice! @pyfisch, are you subscribed to the dev-servo mailing list? I think it’d be nice to send an announcement there with:

  • A link to this PR
  • Format checking is now part of ./mach test-tidy, so unformatted code will fail CI
  • ./mach fmt does the formatting
  • Import ordering has changed (but mach fmt will fix it)

If you prefer I can do it for you :)

@pyfisch
Copy link
Contributor Author

pyfisch commented Nov 7, 2018

Sent an email.

@pyfisch pyfisch deleted the autoformat branch November 7, 2018 19:05
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 7, 2018
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-fails-tidy `./mach test-tidy` reported errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants