mach fmt calls clang-format #24552

paulrouget · 2019-10-25T09:53:00Z

What would be the right way to also integrate this with tidy?

highfive · 2019-10-25T09:53:05Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/testing_commands.py

SimonSapin · 2019-10-25T10:12:42Z

Does clang-format have something like rustfmt --check, which prints a diff and returns an error exit code if it would make any change? ./mach test-tidy does this here:

servo/python/servo/testing_commands.py

Lines 335 to 338 in b7b1b90

    
           self.install_rustfmt() 
        
           rustfmt_failed = self.call_rustup_run(["cargo", "fmt", "--", "--check"]) 
        
           if rustfmt_failed: 
        
               print("Run `./mach fmt` to fix the formatting")

paulrouget · 2019-10-28T06:13:23Z

Does clang-format have something like rustfmt --check

No. clang-format is very limited. I'm gonna have to implement that manually.

Also, note to self, clang-format is not guaranteed to be installed on Mac and Linux.

paulrouget · 2019-10-28T09:54:00Z

There's an option to get a XML output. I will use that.

paulrouget · 2019-10-28T10:49:01Z

@SimonSapin r?

paulrouget · 2019-10-28T11:42:20Z

We will need to install clang-format on the CI machines.

SimonSapin · 2019-10-29T00:14:34Z

Since this requires an externally-installed tool I’d prefer we make this either opt-in or opt-out.

How fast is clang-format? If not very, can we only run it on files that have been changed like tidy

Dependencies for Linux CI tasks are installed here https://github.com/servo/servo/blob/master/etc/taskcluster/docker/build.dockerfile. Modifying this file in your PR should Just Work.

r? @jdm

paulrouget · 2019-10-29T06:22:26Z

Note to self:

make it optin or optout
check speed
version differences on Windows and MacOS (6 vs 9)
update etc/taskcluster/docker/build.dockerfile and etc/taskcluster/macos/Brewfile

SimonSapin · 2019-10-29T07:11:32Z

update etc/taskcluster/docker/build.dockerfile and etc/taskcluster/macos/Brewfile

Do we need both? Isn’t it enough if CI runs clang-format once on all source files?

bors-servo · 2019-10-29T21:09:29Z

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

paulrouget · 2019-10-30T09:27:56Z

I change the logic a bit:

if --no-cpp is passed, clang-format is not used
if clang-format is not available, or has the wrong version (not v6), it skips the check (no failure) and print a message

There's an issue where Visual Studio use clang-format 6, not 9 (the one available on MacOS and probably Linux). And the clang format configurations files are not compatible between versions (that's why I'm checking the clang-format version).

As for the speed, now it only scan files that have changed. It's fast.

I am not changing the CI configuration for osx or linux, as I think it should only be tested on Windows, where the supported version is supposed to be installed. Also, for now, we only check HoloLens code, so it makes sense to support the Windows version. I'm not sure though how my Visual Studio version got clang-format. And I don't know if the CI will have it. Will see from the logs.


        Format CPP code with clang-format

paulrouget · 2019-11-01T05:35:33Z

@jdm r?

jdm · 2019-11-01T05:45:45Z

python/servo/testing_commands.py

+        return False, None, None
+    gitcmd = ['git', 'ls-files']
+    if not all_files:
+            gitcmd.append('-m')


nit: unindent this one level.


        Format support/hololens/

paulrouget · 2019-11-04T06:06:06Z

@bors-servo r=jdm

bors-servo · 2019-11-04T06:06:07Z

📌 Commit 36c1669 has been approved by jdm

bors-servo · 2019-11-04T06:06:12Z

⌛ Testing commit 36c1669 with merge 5caa241...


        Auto merge of #24552 - paulrouget:clangformat, r=jdm

mach fmt calls clang-format Fix #24031 What would be the right way to also integrate this with tidy?

bors-servo · 2019-11-04T06:33:36Z

💔 Test failed - linux-rel-css

CYBAI · 2019-11-04T06:55:42Z

@bors-servo retry

#23220

bors-servo · 2019-11-04T06:55:47Z

⌛ Testing commit 36c1669 with merge e0d81e1...


        Auto merge of #24552 - paulrouget:clangformat, r=jdm

mach fmt calls clang-format Fix #24031 What would be the right way to also integrate this with tidy?

bors-servo · 2019-11-04T08:19:14Z

💔 Test failed - status-taskcluster

CYBAI · 2019-11-04T09:48:15Z

@bors-servo retry

#24622

bors-servo · 2019-11-04T09:48:17Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2019-11-04T09:48:22Z

⌛ Testing commit 36c1669 with merge f626355...


        Auto merge of #24552 - paulrouget:clangformat, r=jdm

mach fmt calls clang-format Fix #24031 What would be the right way to also integrate this with tidy?

bors-servo · 2019-11-04T10:31:06Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing f626355 to master...

highfive assigned asajeffrey Oct 25, 2019

highfive added the S-awaiting-review label Oct 25, 2019

paulrouget force-pushed the paulrouget:clangformat branch from c2ae624 to 7caefb4 Oct 28, 2019

paulrouget force-pushed the paulrouget:clangformat branch from 7caefb4 to 68e3952 Oct 28, 2019

paulrouget mentioned this pull request Oct 28, 2019

Format magicleap CPP code #24566

Open

paulrouget changed the title ~~mach fmt calls clang-format~~ [WIP] mach fmt calls clang-format Oct 29, 2019

highfive added the S-needs-rebase label Oct 29, 2019

paulrouget force-pushed the paulrouget:clangformat branch from 68e3952 to ea0fadc Oct 30, 2019

paulrouget force-pushed the paulrouget:clangformat branch from ea0fadc to b6a8d03 Oct 30, 2019

paulrouget changed the title ~~[WIP] mach fmt calls clang-format~~ mach fmt calls clang-format Oct 30, 2019

Format CPP code with clang-format

70823e3

paulrouget force-pushed the paulrouget:clangformat branch from b6a8d03 to 222ad71 Oct 30, 2019

jdm approved these changes Nov 1, 2019

View changes

python/servo/testing_commands.py Outdated

return False, None, None

gitcmd = ['git', 'ls-files']

if not all_files:

gitcmd.append('-m')

This comment has been minimized.

Sign in to view

Copy link

jdm Nov 1, 2019
Member

nit: unindent this one level.

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Nov 1, 2019

jdm assigned jdm and unassigned asajeffrey Nov 1, 2019

Format support/hololens/

Loading status checks…

36c1669

paulrouget force-pushed the paulrouget:clangformat branch from 222ad71 to 36c1669 Nov 4, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 4, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 4, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 4, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 4, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 4, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 4, 2019

bors-servo merged commit 36c1669 into servo:master Nov 4, 2019
1 of 2 checks passed

1 of 2 checks passed

Taskcluster (pull_request) TaskGroup: failure
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 4, 2019

servo / servo

mach fmt calls clang-format #24552

mach fmt calls clang-format #24552

paulrouget commented Oct 25, 2019

highfive commented Oct 25, 2019

SimonSapin commented Oct 25, 2019

paulrouget commented Oct 28, 2019

paulrouget commented Oct 28, 2019

paulrouget commented Oct 28, 2019

paulrouget commented Oct 28, 2019

SimonSapin commented Oct 29, 2019

paulrouget commented Oct 29, 2019

SimonSapin commented Oct 29, 2019

bors-servo commented Oct 29, 2019

paulrouget commented Oct 30, 2019 •

edited

paulrouget commented Nov 1, 2019

This comment has been minimized.

paulrouget commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

CYBAI commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

CYBAI commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

servo / servo

Join GitHub today

mach fmt calls clang-format #24552

mach fmt calls clang-format #24552

Conversation

paulrouget commented Oct 25, 2019

highfive commented Oct 25, 2019

SimonSapin commented Oct 25, 2019

paulrouget commented Oct 28, 2019

paulrouget commented Oct 28, 2019

paulrouget commented Oct 28, 2019

paulrouget commented Oct 28, 2019

SimonSapin commented Oct 29, 2019

paulrouget commented Oct 29, 2019

SimonSapin commented Oct 29, 2019

bors-servo commented Oct 29, 2019

paulrouget commented Oct 30, 2019 • edited

paulrouget commented Nov 1, 2019

This comment has been minimized.

jdm Nov 1, 2019

paulrouget commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

CYBAI commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

CYBAI commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

bors-servo commented Nov 4, 2019

paulrouget commented Oct 30, 2019 •

edited