git cl format not running correctly on presubmit |
|||||
Issue descriptiongit cl format on the chromium_presubmit builder is passing on a CL with bad formatting. On a closer inspection: [127] chrome-bot@swarm893-c4:(Linux 14.04):/b/swarming/w/ir/cache/builder/chromium_presubmit/src$ /b/swarming/w/ir/recipe-checkout-dir/depot_tools/git_cl.py format --dry-run --presubmit clang-format: /lib/x86_64-linux-gnu/libtinfo.so.5: no version information available (required by clang-format) clang-format: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.20' not found (required by clang-format) clang-format: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by clang-format) Command "/usr/bin/python /b/swarming/w/ir/cache/builder/chromium_presubmit/src/buildtools/clang_format/script/clang-format-diff.py -p0" failed. [1] chrome-bot@swarm893-c4:(Linux 14.04):/b/swarming/w/ir/cache/builder/chromium_presubmit/src$ echo $? 1 It's failing with exit code 1, but presubmit only fails a build if the exit code is 2. There are a couple issues here 1. If git cl format errors out, it still should fail the build. Currently this error is intentionally ignored if the user has a broken clang-format, but this should not be ignored on bots. 2. clang-format dependencies are missing
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/f28ef9887ee23597d4a0b89d068f7b8e377fc876 commit f28ef9887ee23597d4a0b89d068f7b8e377fc876 Author: Ryan Tseng <hinoka@google.com> Date: Tue Dec 04 19:53:08 2018 Add bypass_warnings flag to Commit Format check Right now, if clang-format is broken, a CL will still pass CQ. This adds a flag so that it will fail CQ. Bug: 911708 Change-Id: I2c71b7bc434fc611d51f688266be6a265b80f4da Reviewed-on: https://chromium-review.googlesource.com/c/1361560 Reviewed-by: agrieve <agrieve@chromium.org> Commit-Queue: Ryan Tseng <hinoka@chromium.org> [modify] https://crrev.com/f28ef9887ee23597d4a0b89d068f7b8e377fc876/presubmit_canned_checks.py
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b8921f82290ea4789b7ceaa25f20a1f42edfc73 commit 9b8921f82290ea4789b7ceaa25f20a1f42edfc73 Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Tue Dec 04 21:38:40 2018 Roll src/third_party/depot_tools ba83229a7317..f28ef9887ee2 (1 commits) https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/ba83229a7317..f28ef9887ee2 git log ba83229a7317..f28ef9887ee2 --date=short --no-merges --format='%ad %ae %s' 2018-12-04 hinoka@google.com Add bypass_warnings flag to Commit Format check Created with: gclient setdep -r src/third_party/depot_tools@f28ef9887ee2 The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. BUG= chromium:911708 TBR=agable@chromium.org Change-Id: Ie1df418242a2ae161f09a9515d2d17e860c4bdaa Reviewed-on: https://chromium-review.googlesource.com/c/1361986 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#613706} [modify] https://crrev.com/9b8921f82290ea4789b7ceaa25f20a1f42edfc73/DEPS
,
Dec 4
,
Dec 4
+smaier: You look like the last person to update clang-format. Do you think we can use a statically linked version instead?
,
Dec 5
thakis@ - what are your thoughts here?
,
Dec 5
I thought clang-format is only a presubmit warning and not a presubmit error and hence intentionally doesn't stop the cq.
,
Dec 5
Oh, but clang-format failing to run is a bit of a problem. We could build the clang-format binary against the sysroot, or we could statically link the stdlib (https://bugs.chromium.org/p/chromium/issues/detail?id=907181#c7) which is probably a good idea anyhow (on linux).
,
Dec 5
I'm happy to rebuild clang-format with -static-libgcc -static-libstdc++, but I want to have a way to test whether it will work on these 2 cases where it currently doesn't work (this bug and crbug.com/907181 ). Any ideas?
,
Dec 5
Build locally and look at the output of `ldd clang-format`?
,
Dec 5
There are currently: linux-vdso.so.1 (0x00007ffdae9fe000) libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fcf9b66d000) libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fcf9b453000) librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fcf9b24b000) libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fcf9b047000) libtinfo.so.5 => /lib/x86_64-linux-gnu/libtinfo.so.5 (0x00007fcf9ae1d000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fcf9ab19000) libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fcf9a794000) libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fcf9a57c000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fcf9a1dd000) /lib64/ld-linux-x86-64.so.2 (0x00007fcf9b88a000) We think just making libgcc and libstdc++ static will make it fine? Then leave the other libraries?
,
Dec 5
Passing -DLLVM_ENABLE_TERMINFO=OFF to cmake will remove tinfo, which shouldn't have any downsides. The rest is prbably fine I'm guessing.
,
Dec 5
,
Dec 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/buildtools/+/7d88270de197ebe8b439ab5eb57a4a2a0bb810e0 commit 7d88270de197ebe8b439ab5eb57a4a2a0bb810e0 Author: Sam Maier <smaier@chromium.org> Date: Wed Dec 05 20:05:06 2018 Added statically linked linux binary for clang-format It naturally got a bit bigger: 1957400B now compared to 1862936B before. ldd clang-format: linux-vdso.so.1 (0x00007ffdebff2000) libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f8fe0771000) libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f8fe0557000) librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f8fe034f000) libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f8fe014b000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f8fdfe47000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f8fdfaa8000) Bug: 907181 , 911708 Change-Id: Icf18ff6c5750bd37cd485b3f0e23f4f9f7922b1c [modify] https://crrev.com/7d88270de197ebe8b439ab5eb57a4a2a0bb810e0/linux64/clang-format.sha1
,
Dec 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b5c0a7e5bf7a73942ba783b6180f83a4d63b1e4 commit 9b5c0a7e5bf7a73942ba783b6180f83a4d63b1e4 Author: Sam Maier <smaier@chromium.org> Date: Thu Dec 06 17:35:46 2018 Rolling buildtools 04161ec8..7d88270d Also updates docs on how to build clang-format. Bug: 911708 , 907181 Change-Id: I55b3d9833b030fece1eb52c3b6b53492959582b2 Reviewed-on: https://chromium-review.googlesource.com/c/1365081 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#614404} [modify] https://crrev.com/9b5c0a7e5bf7a73942ba783b6180f83a4d63b1e4/DEPS [modify] https://crrev.com/9b5c0a7e5bf7a73942ba783b6180f83a4d63b1e4/docs/updating_clang_format_binaries.md
,
Dec 6
This should be fixed now. hinoka@ - can you verify that on latest master it works for you? Re-open and re-assign to me if not. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by hinoka@chromium.org
, Dec 4