Would be nice to have a linter to check that only files with a hash-bang may set the executable flag. See #21728
./test/lintFor guidance on contributing, please read CONTRIBUTING.md before opening your pull request.
Perhaps this can be used as a starting point.
It should most probably be ported to Python to become portable, maintainable and readable.
Tested under Linux. I don’t think it will work under macOS (BSD file and stat differences), but I don’t have any Mac to test on :)
0#!/bin/bash
1
2export LC_ALL=C
3
4ALLOWED_PERMISSION_NON_EXECUTABLES=644
5ALLOWED_PERMISSION_EXECUTABLES=755
6ALLOWED_FILENAME_REGEXP='^[a-zA-Z0-9/_.@][a-zA-Z0-9/_.@-]*$'
7
8set -e
9
10# cd to root folder of git repo for git ls-files to work properly
11cd "$(dirname $0)/../.." || exit 1
12
13REPO_FILES=$(git ls-files)
14
15if grep -vE "${ALLOWED_FILENAME_REGEXP}" <<< "${REPO_FILES}"; then
16 echo "^ There exists filenames in the repo which do not match the allowed filename regexp ${ALLOWED_FILENAME_REGEXP}. Failing."
17 exit 1
18fi
19
20EXIT_CODE=0
21
22NON_STANDARD_PERMISSION_FILENAME_PAIRS=$(find ${REPO_FILES} \! -perm ${ALLOWED_PERMISSION_NON_EXECUTABLES} -printf "%m:%p\n")
23for PERMISSION_FILENAME_PAIR in ${NON_STANDARD_PERMISSION_FILENAME_PAIRS}; do
24 PERMISSION=$(cut -f1 -d: <<< "${PERMISSION_FILENAME_PAIR}")
25 FILENAME=$(cut -f2 -d: <<< "${PERMISSION_FILENAME_PAIR}")
26 if [[ ${PERMISSION} == "${ALLOWED_PERMISSION_EXECUTABLES}" ]]; then
27 SHEBANG=$(head -c2 "${FILENAME}")
28 if [[ ${SHEBANG} == "#!" ]]; then
29 continue
30 fi
31 echo "Error: File ${FILENAME} has permission ${ALLOWED_PERMISSION_EXECUTABLES} (executable) and is thus expected to contain a shebang '#!'. Add shebang or do \"chmod ${ALLOWED_PERMISSION_NON_EXECUTABLES} ${FILENAME}\" to make it non-executable."
32 EXIT_CODE=1
33 else
34 echo "Error: File ${FILENAME} has unexpected permission ${PERMISSION}. Do \"chmod ${ALLOWED_PERMISSION_NON_EXECUTABLES} ${FILENAME}\" (if non-executable) or \"chmod ${ALLOWED_PERMISSION_EXECUTABLES} ${FILENAME}\" (if executable)."
35 EXIT_CODE=1
36 fi
37done
38
39exit ${EXIT_CODE}
if [ -x filename ] is a portable (in a POSIX sense) way to check the executable flag of a file.
Here is a first pass at a python version
0#!/usr/bin/env python3
1# Copyright (c) 2021 The Bitcoin Core developers
2# Distributed under the MIT software license, see the accompanying
3# file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5"""
6This checks that all files in the repository have correct names and permissions
7"""
8
9import os
10import re
11import sys
12from subprocess import check_output
13
14ALLOWED_PERMISSION_NON_EXECUTABLES = 644
15ALLOWED_PERMISSION_EXECUTABLES = 755
16ALLOWED_FILENAME_REGEXP = '^[a-zA-Z0-9/_.@][a-zA-Z0-9/_.@-]*$'
17CMD_REPO_FILES = 'git ls-files'
18
19
20def check_files():
21 files = check_output(CMD_REPO_FILES, shell=True).decode('utf8').strip().split('\n')
22 filename_regex = re.compile(ALLOWED_FILENAME_REGEXP)
23 for file in files:
24 # Check file name
25 if not filename_regex.match(file):
26 print(f"'{file}' does not not match the allowed filename regexp. Failing.")
27 sys.exit(1)
28
29 # Check file permissions
30 file_perms = int(oct(os.stat(file).st_mode)[-3:])
31 if file_perms == ALLOWED_PERMISSION_EXECUTABLES:
32 shebang = open(file, "r").readline(2)
33 if shebang == '#!':
34 continue
35 print(f"File '{file}' has permission {ALLOWED_PERMISSION_EXECUTABLES} (executable) and is thus expected to contain a shebang '#!'. Add shebang or do \"chmod {ALLOWED_PERMISSION_NON_EXECUTABLES} {file}\" to make it non-executable.")
36 sys.exit(1)
37 elif file_perms == ALLOWED_PERMISSION_NON_EXECUTABLES:
38 continue
39 else:
40 print(f"File '{file}' has unexpected permission {file_perms}. Do \"chmod {ALLOWED_PERMISSION_NON_EXECUTABLES} {file}\" (if non-executable) or \"chmod {ALLOWED_PERMISSION_EXECUTABLES} {file}\" (if executable).")
41 sys.exit(1)
42
43
44def main():
45 check_files()
46
47
48if __name__ == "__main__":
49 main()
You likely already found this in your testing @practicalswift, but it seems that https://github.com/bitcoin/bitcoin/blob/master/contrib/gitian-descriptors/assign_DISTNAME currently is executable, but missing a shebang line. I could not find any other files with an issue.
Let me know if the above looks anywhere close to acceptable, and I will look into what I need to do to open a proper PR. It would be my first code contribution to Bitcoin Core :)
Nice! Your Python script is portable, maintainable and readable which cannot be said about my hacky bash script :) Thanks for taking on the task!
Name is say test/lint/lint-file-permissions.py and call it from a shell wrapper test/lint/lint-file-permissions.sh (to make it automatically invoked by test/lint/lint-all.sh) like this …
0#!/usr/bin/env bash
1
2export LC_ALL=C
3
4set -e
5cd "$(dirname $0)/../.."
6test/lint/lint-file-permissions.py
… and we should be ready to go! I’m ready to review :)
Below is a small diff which addresses three small nits - feel free to use or ignore :)
rb instead of r in order to not fail on non-ASCII files (locale dependent) 0diff --git a/test/lint/lint-file-permissions.py b/test/lint/lint-file-permissions.py
1index eb4021fc3..78278e27b 100755
2--- a/test/lint/lint-file-permissions.py
3+++ b/test/lint/lint-file-permissions.py
4@@ -21,29 +21,31 @@ CMD_REPO_FILES = 'git ls-files'
5 def check_files():
6 files = check_output(CMD_REPO_FILES, shell=True).decode('utf8').strip().split('\n')
7 filename_regex = re.compile(ALLOWED_FILENAME_REGEXP)
8+ exit_code = 0
9 for file in files:
10 # Check file name
11 if not filename_regex.match(file):
12- print(f"'{file}' does not not match the allowed filename regexp. Failing.")
13- sys.exit(1)
14+ print(f"'{file}' does not not match the allowed filename regexp ('{ALLOWED_FILENAME_REGEXP}').")
15+ exit_code = 1
16
17 # Check file permissions
18 file_perms = int(oct(os.stat(file).st_mode)[-3:])
19 if file_perms == ALLOWED_PERMISSION_EXECUTABLES:
20- shebang = open(file, "r").readline(2)
21- if shebang == '#!':
22+ shebang = open(file, "rb").read(2)
23+ if shebang == b'#!':
24 continue
25 print(f"File '{file}' has permission {ALLOWED_PERMISSION_EXECUTABLES} (executable) and is thus expected to contain a shebang '#!'. Add shebang or do \"chmod {ALLOWED_PERMISSION_NON_EXECUTABLES} {file}\" to make it non-executable.")
26- sys.exit(1)
27+ exit_code = 1
28 elif file_perms == ALLOWED_PERMISSION_NON_EXECUTABLES:
29 continue
30 else:
31 print(f"File '{file}' has unexpected permission {file_perms}. Do \"chmod {ALLOWED_PERMISSION_NON_EXECUTABLES} {file}\" (if non-executable) or \"chmod {ALLOWED_PERMISSION_EXECUTABLES} {file}\" (if executable).")
32- sys.exit(1)
33+ exit_code = 1
34+ return exit_code
35
36
37 def main():
38- check_files()
39+ sys.exit(check_files())
40
41
42 if __name__ == "__main__":
Don’t forget to run black on the file before submitting the PR :)
It might be good to also check that files with the bang do have the +x flag set. See also https://github.com/bitcoin/bitcoin/pull/16807/files#r345547050
+1
@windsok, git grep -lI '^#!' might be helpful to quickly get a list of the 305 shebang files in the repo :)
Thanks @MarcoFalke & @practicalswift ! I will do all of this and get a PR together :)
I also noticed that we have this existing lint test which is checking that executable .sh and .py files have a specific shebang line. I am thinking that it probably makes sense to also move these tests into this new python test file, so that all similar tests are located in the same test file. Does that make sense to you?
https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-shebang.sh
We also have this existing lint test which is checking filenames in the repo. I am thinking that we should probably either also consolidate this test into the new python test file, or we should remove the filename checking and leave the new tests to be purely related to permissions and shebang lines?
https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-filenames.sh
Good idea!
Generally: the more linting functionality we can move from non-maintainable shell scripts to maintainable Python scripts the better :)