From 00c3e0e8ec20fd2a0be64f2b348b13201034a2a5 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Sat, 12 Sep 2020 13:30:09 +0200 Subject: [PATCH] codeowners: fix directory patterns The previous version used an incorrect /dir1/dir2 pattern to match the content of dir2. The correct pattern should be /dir1/dir2/ (with the trailing slash). This commit fixes these patterns. Regarding codeowners.py: 'git ls-files' can not be used to correctly implement the logic of CODEOWNERS file patterns, since it doesn't distinguish between /path/* and /path/. The former pattern in CODEOWNERS file should only match the files inside /path/, while the latter also matches files in nested directories. Because of this, the logic for evaluating patterns is re-implemented, by converting CODEOWNERS patterns into regular expressions. Gitlab CODEOWNERS parsing code was used as a reference, in addition to the approach for converting glob patterns into regular expressions proposed in https://stackoverflow.com/a/29354254. --- .gitlab/CODEOWNERS | 260 +++++++++++++++++++++++++++----------------- tools/codeowners.py | 110 +++++++++++++++---- 2 files changed, 251 insertions(+), 119 deletions(-) diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS index aa7317a6de..f41ecc35f4 100644 --- a/.gitlab/CODEOWNERS +++ b/.gitlab/CODEOWNERS @@ -1,107 +1,167 @@ +# For the syntax of this file, see: +# +# https://docs.gitlab.com/ee/user/project/code_owners.html#the-syntax-of-code-owners-files +# +# If more than one rule matches a given file, the latest rule is used. +# The file should be generally kept sorted, except when it is necessary +# to use a different order due to the fact above. In that case, use +# '# sort-order-reset' comment line to reset the sort order. +# +# Recipes for a few common cases: +# +# 1. Specific directory with all its contents: +# +# /components/app_trace/ +# +# Note the trailing slash! +# +# 2. File with certain extension in any subdirectory of a certain directory: +# +# /examples/**/*.py +# +# This includes an *.py files in /examples/ directory as well. +# +# 3. Contents of a directory with a certain name, anywhere in the tree: +# +# test_*_host/ +# +# Will match everything under components/efuse/test_efuse_host/, +# components/heap/test_multi_heap_host/, components/lwip/test_afl_host/, etc. +# +# 4. Same as above, except limited to a specific place in the tree: +# +# /components/esp32*/ +# +# Matches everything under /components/esp32, /components/esp32s2, etc. +# Doesn't match /tools/some-test/components/esp32s5. +# +# 5. Specific file: +# +# /tools/tools.json +# +# 6. File with a certain name anywhere in the tree +# +# .gitignore +# + * @esp-idf-codeowners/other -components/app_trace @esp-idf-codeowners/tools -components/app_update @esp-idf-codeowners/system @esp-idf-codeowners/app-utilities -components/asio @esp-idf-codeowners/network -components/bootloader*/** @esp-idf-codeowners/system @esp-idf-codeowners/security -components/bt @esp-idf-codeowners/bluetooth -components/cbor @esp-idf-codeowners/app-utilities -components/coap @esp-idf-codeowners/app-utilities -components/console @esp-idf-codeowners/system @esp-idf-codeowners/app-utilities -components/cxx @esp-idf-codeowners/system -components/driver @esp-idf-codeowners/peripherals -components/efuse @esp-idf-codeowners/system -components/esp32* @esp-idf-codeowners/system -components/esp_adc_cal @esp-idf-codeowners/peripherals -components/esp_common @esp-idf-codeowners/system -components/esp_eth @esp-idf-codeowners/network -components/esp_event @esp-idf-codeowners/system -components/esp_gdbstub @esp-idf-codeowners/tools -components/esp_hid @esp-idf-codeowners/bluetooth -components/esp_http_client @esp-idf-codeowners/app-utilities -components/esp_http_server @esp-idf-codeowners/app-utilities -components/esp_https_ota @esp-idf-codeowners/app-utilities -components/esp_https_server @esp-idf-codeowners/app-utilities -components/esp_ipc @esp-idf-codeowners/system -components/esp_local_ctrl @esp-idf-codeowners/app-utilities -components/esp_netif @esp-idf-codeowners/network -components/esp_ringbuf @esp-idf-codeowners/system -components/esp_rom @esp-idf-codeowners/system -components/esp_serial_slave_link @esp-idf-codeowners/peripherals -components/esp_system @esp-idf-codeowners/system -components/esp_timer @esp-idf-codeowners/system -components/esp-tls @esp-idf-codeowners/app-utilities -components/esp_websocket_client @esp-idf-codeowners/network -components/esp_wifi @esp-idf-codeowners/wifi -components/espcoredump @esp-idf-codeowners/tools -components/esptool_py @esp-idf-codeowners/tools -components/expat @esp-idf-codeowners/app-utilities -components/fatfs @esp-idf-codeowners/storage -components/freemodbus @esp-idf-codeowners/peripherals -components/freertos @esp-idf-codeowners/system -components/heap @esp-idf-codeowners/system -components/idf_test @esp-idf-codeowners/ci -components/jsmn @esp-idf-codeowners/app-utilities -components/json @esp-idf-codeowners/app-utilities -components/libsodium @esp-idf-codeowners/security -components/log @esp-idf-codeowners/system -components/lwip @esp-idf-codeowners/lwip -components/mbedtls @esp-idf-codeowners/app-utilities @esp-idf-codeowners/security -components/mdns @esp-idf-codeowners/network -components/mqtt @esp-idf-codeowners/network -components/newlib @esp-idf-codeowners/system @esp-idf-codeowners/tools -components/nghttp @esp-idf-codeowners/app-utilities -components/nvs_flash @esp-idf-codeowners/storage -components/openssl @esp-idf-codeowners/network -components/partition_table @esp-idf-codeowners/system -components/perfmon @esp-idf-codeowners/tools -components/protobuf-c @esp-idf-codeowners/app-utilities -components/protocomm @esp-idf-codeowners/app-utilities -components/pthread @esp-idf-codeowners/system -components/sdmmc @esp-idf-codeowners/storage -components/soc @esp-idf-codeowners/peripherals -components/spi_flash @esp-idf-codeowners/peripherals -components/spiffs @esp-idf-codeowners/storage -components/tcp_transport @esp-idf-codeowners/network -components/tcpip_adapter @esp-idf-codeowners/network -components/tinyusb @esp-idf-codeowners/peripherals -components/ulp @esp-idf-codeowners/system -components/unity @esp-idf-codeowners/ci -components/vfs @esp-idf-codeowners/storage -components/wear_levelling @esp-idf-codeowners/storage -components/wifi_provisioning @esp-idf-codeowners/app-utilities -components/wpa_supplicant @esp-idf-codeowners/wifi -components/xtensa @esp-idf-codeowners/system +/.* @esp-idf-codeowners/tools +/.gitlab-ci.yml @esp-idf-codeowners/ci +/.readthedocs.yml @esp-idf-codeowners/docs +/CMakeLists.txt @esp-idf-codeowners/build-config +/Kconfig @esp-idf-codeowners/build-config +/add_path.sh @esp-idf-codeowners/tools +/export.* @esp-idf-codeowners/tools +/install.* @esp-idf-codeowners/tools +/sdkconfig.rename @esp-idf-codeowners/build-config -docs @esp-idf-codeowners/docs +# sort-order-reset -examples/**/*.py @esp-idf-codeowners/ci @esp-idf-codeowners/tools +/components/app_trace/ @esp-idf-codeowners/tools +/components/app_update/ @esp-idf-codeowners/system @esp-idf-codeowners/app-utilities +/components/asio/ @esp-idf-codeowners/network +/components/bootloader*/ @esp-idf-codeowners/system @esp-idf-codeowners/security +/components/bt/ @esp-idf-codeowners/bluetooth +/components/cbor/ @esp-idf-codeowners/app-utilities +/components/coap/ @esp-idf-codeowners/app-utilities +/components/console/ @esp-idf-codeowners/system @esp-idf-codeowners/app-utilities +/components/cxx/ @esp-idf-codeowners/system +/components/driver/ @esp-idf-codeowners/peripherals +/components/efuse/ @esp-idf-codeowners/system +/components/esp32*/ @esp-idf-codeowners/system +/components/esp_adc_cal/ @esp-idf-codeowners/peripherals +/components/esp_common/ @esp-idf-codeowners/system +/components/esp_eth/ @esp-idf-codeowners/network +/components/esp_event/ @esp-idf-codeowners/system +/components/esp_gdbstub/ @esp-idf-codeowners/tools +/components/esp_hid/ @esp-idf-codeowners/bluetooth +/components/esp_http_client/ @esp-idf-codeowners/app-utilities +/components/esp_http_server/ @esp-idf-codeowners/app-utilities +/components/esp_https_ota/ @esp-idf-codeowners/app-utilities +/components/esp_https_server/ @esp-idf-codeowners/app-utilities +/components/esp_ipc/ @esp-idf-codeowners/system +/components/esp_local_ctrl/ @esp-idf-codeowners/app-utilities +/components/esp_netif/ @esp-idf-codeowners/network +/components/esp_ringbuf/ @esp-idf-codeowners/system +/components/esp_rom/ @esp-idf-codeowners/system +/components/esp_serial_slave_link/ @esp-idf-codeowners/peripherals +/components/esp_system/ @esp-idf-codeowners/system +/components/esp_timer/ @esp-idf-codeowners/system +/components/esp-tls/ @esp-idf-codeowners/app-utilities +/components/esp_websocket_client/ @esp-idf-codeowners/network +/components/esp_wifi/ @esp-idf-codeowners/wifi +/components/espcoredump/ @esp-idf-codeowners/tools +/components/esptool_py/ @esp-idf-codeowners/tools +/components/expat/ @esp-idf-codeowners/app-utilities +/components/fatfs/ @esp-idf-codeowners/storage +/components/freemodbus/ @esp-idf-codeowners/peripherals +/components/freertos/ @esp-idf-codeowners/system +/components/heap/ @esp-idf-codeowners/system +/components/idf_test/ @esp-idf-codeowners/ci +/components/jsmn/ @esp-idf-codeowners/app-utilities +/components/json/ @esp-idf-codeowners/app-utilities +/components/libsodium/ @esp-idf-codeowners/security +/components/log/ @esp-idf-codeowners/system +/components/lwip/ @esp-idf-codeowners/lwip +/components/mbedtls/ @esp-idf-codeowners/app-utilities @esp-idf-codeowners/security +/components/mdns/ @esp-idf-codeowners/network +/components/mqtt/ @esp-idf-codeowners/network +/components/newlib/ @esp-idf-codeowners/system @esp-idf-codeowners/tools +/components/nghttp/ @esp-idf-codeowners/app-utilities +/components/nvs_flash/ @esp-idf-codeowners/storage +/components/openssl/ @esp-idf-codeowners/network +/components/partition_table/ @esp-idf-codeowners/system +/components/perfmon/ @esp-idf-codeowners/tools +/components/protobuf-c/ @esp-idf-codeowners/app-utilities +/components/protocomm/ @esp-idf-codeowners/app-utilities +/components/pthread/ @esp-idf-codeowners/system +/components/sdmmc/ @esp-idf-codeowners/storage +/components/soc/ @esp-idf-codeowners/peripherals +/components/spi_flash/ @esp-idf-codeowners/peripherals +/components/spiffs/ @esp-idf-codeowners/storage +/components/tcp_transport/ @esp-idf-codeowners/network +/components/tcpip_adapter/ @esp-idf-codeowners/network +/components/tinyusb/ @esp-idf-codeowners/peripherals +/components/ulp/ @esp-idf-codeowners/system +/components/unity/ @esp-idf-codeowners/ci +/components/vfs/ @esp-idf-codeowners/storage +/components/wear_levelling/ @esp-idf-codeowners/storage +/components/wifi_provisioning/ @esp-idf-codeowners/app-utilities +/components/wpa_supplicant/ @esp-idf-codeowners/wifi +/components/xtensa/ @esp-idf-codeowners/system -examples/bluetooth @esp-idf-codeowners/bluetooth -examples/build_system @esp-idf-codeowners/build-config -examples/common_components @esp-idf-codeowners/system -examples/cxx @esp-idf-codeowners/system -examples/ethernet @esp-idf-codeowners/network -examples/get-started @esp-idf-codeowners/system -examples/mesh @esp-idf-codeowners/wifi -examples/peripherals @esp-idf-codeowners/peripherals -examples/protocols @esp-idf-codeowners/network @esp-idf-codeowners/app-utilities -examples/provisioning @esp-idf-codeowners/app-utilities -examples/security @esp-idf-codeowners/security -examples/storage @esp-idf-codeowners/storage -examples/system @esp-idf-codeowners/system -examples/wifi @esp-idf-codeowners/wifi +/docs/ @esp-idf-codeowners/docs -make @esp-idf-codeowners/build-config +/examples/**/*.py @esp-idf-codeowners/ci @esp-idf-codeowners/tools -tools @esp-idf-codeowners/tools -tools/*_apps.py @esp-idf-codeowners/ci -tools/ble @esp-idf-codeowners/app-utilities -tools/catch @esp-idf-codeowners/ci -tools/ci @esp-idf-codeowners/ci -tools/cmake @esp-idf-codeowners/build-config -tools/esp_prov @esp-idf-codeowners/app-utilities -tools/find_build_apps @esp-idf-codeowners/ci -tools/kconfig* @esp-idf-codeowners/build-config -tools/ldgen @esp-idf-codeowners/build-config -tools/mass_mfg @esp-idf-codeowners/app-utilities +/examples/bluetooth/ @esp-idf-codeowners/bluetooth +/examples/build_system/ @esp-idf-codeowners/build-config +/examples/common_components/ @esp-idf-codeowners/system +/examples/cxx/ @esp-idf-codeowners/system +/examples/ethernet/ @esp-idf-codeowners/network +/examples/get-started/ @esp-idf-codeowners/system +/examples/mesh/ @esp-idf-codeowners/wifi +/examples/peripherals/ @esp-idf-codeowners/peripherals +/examples/protocols/ @esp-idf-codeowners/network @esp-idf-codeowners/app-utilities +/examples/provisioning/ @esp-idf-codeowners/app-utilities +/examples/security/ @esp-idf-codeowners/security +/examples/storage/ @esp-idf-codeowners/storage +/examples/system/ @esp-idf-codeowners/system +/examples/wifi/ @esp-idf-codeowners/wifi + +/make/ @esp-idf-codeowners/build-config + +/tools/ @esp-idf-codeowners/tools +/tools/*_apps.py @esp-idf-codeowners/ci +/tools/ble/ @esp-idf-codeowners/app-utilities +/tools/catch/ @esp-idf-codeowners/ci +/tools/ci/ @esp-idf-codeowners/ci +/tools/cmake/ @esp-idf-codeowners/build-config +/tools/esp_prov/ @esp-idf-codeowners/app-utilities +/tools/find_build_apps/ @esp-idf-codeowners/ci +/tools/kconfig*/ @esp-idf-codeowners/build-config +/tools/ldgen/ @esp-idf-codeowners/build-config +/tools/mass_mfg/ @esp-idf-codeowners/app-utilities + +requirements.txt @esp-idf-codeowners/tools diff --git a/tools/codeowners.py b/tools/codeowners.py index 7cf1ab4944..34e9365b45 100755 --- a/tools/codeowners.py +++ b/tools/codeowners.py @@ -17,17 +17,79 @@ # limitations under the License. import argparse -from collections import defaultdict import os +import re import subprocess import sys + CODEOWNERS_PATH = os.path.join(os.path.dirname(__file__), "..", ".gitlab", "CODEOWNERS") CODEOWNER_GROUP_PREFIX = "@esp-idf-codeowners/" +def get_all_files(): + """ + Get list of all file paths in the repository. + """ + idf_root = os.path.join(os.path.dirname(__file__), "..") + # only split on newlines, since file names may contain spaces + return subprocess.check_output(["git", "ls-files"], cwd=idf_root).decode("utf-8").strip().split('\n') + + +def pattern_to_regex(pattern): + """ + Convert the CODEOWNERS path pattern into a regular expression string. + """ + orig_pattern = pattern # for printing errors later + + # Replicates the logic from normalize_pattern function in Gitlab ee/lib/gitlab/code_owners/file.rb: + if not pattern.startswith('/'): + pattern = '/**/' + pattern + if pattern.endswith('/'): + pattern = pattern + '**/*' + + # Convert the glob pattern into a regular expression: + # first into intermediate tokens + pattern = (pattern.replace('**/', ':REGLOB:') + .replace('**', ':INVALID:') + .replace('*', ':GLOB:') + .replace('.', ':DOT:') + .replace('?', ':ANY:')) + + if pattern.find(':INVALID:') >= 0: + raise ValueError("Likely invalid pattern '{}': '**' should be followed by '/'".format(orig_pattern)) + + # then into the final regex pattern: + re_pattern = (pattern.replace(':REGLOB:', '(?:.*/)?') + .replace(':GLOB:', '[^/]*') + .replace(':DOT:', '[.]') + .replace(':ANY:', '.') + '$') + if re_pattern.startswith('/'): + re_pattern = '^' + re_pattern + + return re_pattern + + +def files_by_regex(all_files, regex): + """ + Return all files in the repository matching the given regular expresion. + """ + return [file for file in all_files if regex.search('/' + file)] + + +def files_by_pattern(all_files, pattern=None): + """ + Return all the files in the repository matching the given CODEOWNERS pattern. + """ + if not pattern: + return all_files + + return files_by_regex(all_files, re.compile(pattern_to_regex(pattern))) + + def action_identify(args): best_match = [] + all_files = get_all_files() with open(CODEOWNERS_PATH) as f: for line in f: line = line.strip() @@ -36,19 +98,23 @@ def action_identify(args): tokens = line.split() path_pattern = tokens[0] owners = tokens[1:] - files = files_by_pattern(path_pattern) + files = files_by_pattern(all_files, path_pattern) if args.path in files: best_match = owners for owner in best_match: print(owner) -def files_by_pattern(pattern=None): - args = ["git", "ls-files"] - if pattern: - args.append(pattern) - idf_root = os.path.join(os.path.dirname(__file__), "..") - return subprocess.check_output(args, cwd=idf_root).decode("utf-8").split() +def action_test_pattern(args): + re_pattern = pattern_to_regex(args.pattern) + + if args.regex: + print(re_pattern) + return + + files = files_by_regex(get_all_files(), re.compile(re_pattern)) + for f in files: + print(f) def action_ci_check(args): @@ -57,12 +123,15 @@ def action_ci_check(args): def add_error(msg): errors.append("Error at CODEOWNERS:{}: {}".format(line_no, msg)) - files_by_owner = defaultdict(int) + all_files = get_all_files() prev_path_pattern = "" with open(CODEOWNERS_PATH) as f: for line_no, line in enumerate(f, start=1): # Skip empty lines and comments line = line.strip() + if line.startswith("# sort-order-reset"): + prev_path_pattern = "" + if not line or line.startswith("#"): continue @@ -75,26 +144,19 @@ def action_ci_check(args): # Check that the file is sorted by path patterns path_pattern_for_cmp = path_pattern.replace("-", "_") # ignore difference between _ and - for ordering - if path_pattern_for_cmp < prev_path_pattern: + if prev_path_pattern and path_pattern_for_cmp < prev_path_pattern: add_error("file is not sorted: {} < {}".format(path_pattern_for_cmp, prev_path_pattern)) prev_path_pattern = path_pattern_for_cmp # Check that the pattern matches at least one file - files = files_by_pattern(path_pattern) + files = files_by_pattern(all_files, path_pattern) if not files: add_error("no files matched by pattern {}".format(path_pattern)) - # Count the number of files per owner for o in owners: # Sanity-check the owner group name if not o.startswith(CODEOWNER_GROUP_PREFIX): add_error("owner {} doesn't start with {}".format(o, CODEOWNER_GROUP_PREFIX)) - files_by_owner[o] += len(files) - - owners_sorted = sorted([(owner, cnt) for owner, cnt in files_by_owner.items()], key=lambda p: p[0]) - print("File count per owner (not including submodules):") - for owner, cnt in owners_sorted: - print("{}: {} files".format(owner, cnt)) if not errors: print("No errors found.") @@ -110,17 +172,27 @@ def main(): sys.argv[0], description="Internal helper script for working with the CODEOWNERS file." ) subparsers = parser.add_subparsers(dest="action") + identify = subparsers.add_parser( "identify", - help="Lists the owners of the specified path within IDF." + help="List the owners of the specified path within IDF." "This command doesn't support files inside submodules, or files not added to git repository.", ) identify.add_argument("path", help="Path of the file relative to the root of the repository") + subparsers.add_parser( "ci-check", help="Check CODEOWNERS file: every line should match at least one file, sanity-check group names, " "check that the file is sorted by paths", ) + + test_pattern = subparsers.add_parser( + "test-pattern", + help="Print files in the repository for a given CODEOWNERS pattern. Useful when adding new rules." + ) + test_pattern.add_argument("--regex", action="store_true", help="Print the equivalent regular expression instead of the file list.") + test_pattern.add_argument("pattern", help="Path pattern to get the list of files for") + args = parser.parse_args() if args.action is None: