From 76aba1f0460fde2115842e8480d700f1e5418138 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Tue, 11 Feb 2020 13:28:16 +0000 Subject: [PATCH] ci: Add a style check pass We should at least try to get the style check done by a machine, as opposed to have humans go through merge requests with coding style nitpicks. --- .clang-format | 11 +++ .gitlab-ci.yml | 21 ++++- .gitlab-ci/clang-format-diff.py | 133 +++++++++++++++++++++++++++++ .gitlab-ci/fedora.Dockerfile | 1 + .gitlab-ci/run-style-check-diff.sh | 34 ++++++++ 5 files changed, 197 insertions(+), 3 deletions(-) create mode 100644 .clang-format create mode 100755 .gitlab-ci/clang-format-diff.py create mode 100755 .gitlab-ci/run-style-check-diff.sh diff --git a/.clang-format b/.clang-format new file mode 100644 index 0000000000..13fd0fbd44 --- /dev/null +++ b/.clang-format @@ -0,0 +1,11 @@ +# See https://wiki.apertis.org/Guidelines/Coding_conventions#Code_formatting +BasedOnStyle: GNU +AlwaysBreakAfterDefinitionReturnType: All +BreakBeforeBinaryOperators: None +BinPackParameters: false +SpaceAfterCStyleCast: true +# Our column limit is actually 80, but setting that results in clang-format +# making a lot of dubious hanging-indent choices; disable it and assume the +# developer will line wrap appropriately. clang-format will still check +# existing hanging indents. +ColumnLimit: 0 diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 26e4c0ea10..58db3d7a4e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,4 +1,5 @@ stages: + - style-check - build - flatpak - deploy @@ -19,8 +20,22 @@ variables: G_MESSAGES_DEBUG: "all" CCACHE_DISABLE: "true" +.only-default: + only: + - branches + except: + - tags + +style-check-diff: + extends: .only-default + image: registry.gitlab.gnome.org/gnome/gtk/fedora:v13 + stage: style-check + allow_failure: true + script: + - .gitlab-ci/run-style-check-diff.sh + fedora-x86_64: - image: registry.gitlab.gnome.org/gnome/gtk/fedora:v12 + image: registry.gitlab.gnome.org/gnome/gtk/fedora:v13 stage: build variables: EXTRA_MESON_FLAGS: "--buildtype=debug --default-library=both" @@ -52,7 +67,7 @@ fedora-x86_64: <<: *cache-paths release-build: - image: registry.gitlab.gnome.org/gnome/gtk/fedora:v12 + image: registry.gitlab.gnome.org/gnome/gtk/fedora:v13 stage: build variables: EXTRA_MESON_FLAGS: "--buildtype=release" @@ -151,7 +166,7 @@ flatpak-master:icon-browser: <<: *flatpak-master pages: - image: registry.gitlab.gnome.org/gnome/gtk/fedora:v12 + image: registry.gitlab.gnome.org/gnome/gtk/fedora:v13 stage: deploy variables: EXTRA_MESON_FLAGS: "--buildtype=release" diff --git a/.gitlab-ci/clang-format-diff.py b/.gitlab-ci/clang-format-diff.py new file mode 100755 index 0000000000..3fb776c592 --- /dev/null +++ b/.gitlab-ci/clang-format-diff.py @@ -0,0 +1,133 @@ +#!/usr/bin/env python3 +# +# === clang-format-diff.py - ClangFormat Diff Reformatter ---*- python -*-=== # +# +# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +# See https://llvm.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +# ===---------------------------------------------------------------------=== # + +""" +This script reads input from a unified diff and reformats all the changed +lines. This is useful to reformat all the lines touched by a specific patch. +Example usage for git/svn users: + + git diff -U0 --no-color HEAD^ | clang-format-diff.py -p1 -i + svn diff --diff-cmd=diff -x-U0 | clang-format-diff.py -i + +""" +from __future__ import absolute_import, division, print_function + +import argparse +import difflib +import re +import subprocess +import sys + +if sys.version_info.major >= 3: + from io import StringIO +else: + from io import BytesIO as StringIO + + +def main(): + parser = argparse.ArgumentParser( + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument('-i', action='store_true', default=False, + help='apply edits to files instead of displaying a ' + 'diff') + parser.add_argument('-p', metavar='NUM', default=0, + help='strip the smallest prefix containing P slashes') + parser.add_argument('-regex', metavar='PATTERN', default=None, + help='custom pattern selecting file paths to reformat ' + '(case sensitive, overrides -iregex)') + parser.add_argument('-iregex', metavar='PATTERN', + default=r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hh|hpp|m|mm|inc' + r'|js|ts|proto|protodevel|java|cs)', + help='custom pattern selecting file paths to reformat ' + '(case insensitive, overridden by -regex)') + parser.add_argument('-sort-includes', action='store_true', default=False, + help='let clang-format sort include blocks') + parser.add_argument('-v', '--verbose', action='store_true', + help='be more verbose, ineffective without -i') + parser.add_argument('-style', + help='formatting style to apply (LLVM, Google, ' + 'Chromium, Mozilla, WebKit)') + parser.add_argument('-binary', default='clang-format', + help='location of binary to use for clang-format') + args = parser.parse_args() + + # Extract changed lines for each file. + filename = None + lines_by_file = {} + for line in sys.stdin: + match = re.search(r'^\+\+\+\ (.*?/){%s}(\S*)' % args.p, line) + if match: + filename = match.group(2) + if filename is None: + continue + + if args.regex is not None: + if not re.match('^%s$' % args.regex, filename): + continue + else: + if not re.match('^%s$' % args.iregex, filename, re.IGNORECASE): + continue + + match = re.search(r'^@@.*\+(\d+)(,(\d+))?', line) + if match: + start_line = int(match.group(1)) + line_count = 1 + if match.group(3): + line_count = int(match.group(3)) + if line_count == 0: + continue + end_line = start_line + line_count - 1 + lines_by_file.setdefault(filename, []).extend( + ['-lines', str(start_line) + ':' + str(end_line)]) + + # Reformat files containing changes in place. + # We need to count amount of bytes generated in the output of + # clang-format-diff. If clang-format-diff doesn't generate any bytes it + # means there is nothing to format. + format_line_counter = 0 + for filename, lines in lines_by_file.items(): + if args.i and args.verbose: + print('Formatting {}'.format(filename)) + command = [args.binary, filename] + if args.i: + command.append('-i') + if args.sort_includes: + command.append('-sort-includes') + command.extend(lines) + if args.style: + command.extend(['-style', args.style]) + p = subprocess.Popen(command, + stdout=subprocess.PIPE, + stderr=None, + stdin=subprocess.PIPE, + universal_newlines=True) + stdout, _ = p.communicate() + if p.returncode != 0: + sys.exit(p.returncode) + + if not args.i: + with open(filename) as f: + code = f.readlines() + formatted_code = StringIO(stdout).readlines() + diff = difflib.unified_diff(code, formatted_code, + filename, filename, + '(before formatting)', + '(after formatting)') + diff_string = ''.join(diff) + if diff_string: + format_line_counter += sys.stdout.write(diff_string) + + if format_line_counter > 0: + sys.exit(1) + + +if __name__ == '__main__': + main() diff --git a/.gitlab-ci/fedora.Dockerfile b/.gitlab-ci/fedora.Dockerfile index 8c96f33222..1a0ca24dec 100644 --- a/.gitlab-ci/fedora.Dockerfile +++ b/.gitlab-ci/fedora.Dockerfile @@ -8,6 +8,7 @@ RUN dnf -y install \ cairo-devel \ cairo-gobject-devel \ ccache \ + clang \ colord-devel \ cups-devel \ dbus-daemon \ diff --git a/.gitlab-ci/run-style-check-diff.sh b/.gitlab-ci/run-style-check-diff.sh new file mode 100755 index 0000000000..e6f54f93c6 --- /dev/null +++ b/.gitlab-ci/run-style-check-diff.sh @@ -0,0 +1,34 @@ +#!/bin/bash + +set +e + +# We need to add a new remote for the upstream master, since this script could +# be running in a personal fork of the repository which has out of date branches. +git remote add upstream https://gitlab.gnome.org/GNOME/gtk.git +git fetch upstream + +# Work out the newest common ancestor between the detached HEAD that this CI job +# has checked out, and the upstream target branch (which will typically be +# `upstream/master` or `upstream/gtk-3-24`). +# +# `${CI_MERGE_REQUEST_TARGET_BRANCH_NAME}` is only defined if we’re running in +# a merge request pipeline; fall back to `${CI_DEFAULT_BRANCH}` otherwise. +newest_common_ancestor_sha=$(diff --old-line-format='' --new-line-format='' <(git rev-list --first-parent upstream/${CI_MERGE_REQUEST_TARGET_BRANCH_NAME:-${CI_DEFAULT_BRANCH}}) <(git rev-list --first-parent HEAD) | head -1) +git diff -U0 --no-color "${newest_common_ancestor_sha}" | .gitlab-ci/clang-format-diff.py -binary "clang-format" -p1 + +# The style check is not infallible. The clang-format configuration cannot +# perfectly describe GTK’s coding style: in particular, it cannot align +# function arguments. The documented coding style for GTK takes priority over +# clang-format suggestions. Hopefully we can eventually improve clang-format to +# be configurable enough for our coding style. That’s why this CI check is OK +# to fail: the idea is that people can look through the output and ignore it if +# it’s wrong. (That situation can also happen if someone touches pre-existing +# badly formatted code and it doesn’t make sense to tidy up the wider coding +# style with the changes they’re making.) +echo "" +echo "Note that clang-format output is advisory and cannot always match the" +echo "GTK coding style, documented at:" +echo " https://gitlab.gnome.org/GNOME/gtk/blob/master/docs/CODING-STYLE" +echo "Warnings from this tool can be ignored in favour of the documented " +echo "coding style, or in favour of matching the style of existing" +echo "surrounding code."