Use annotations to give feedback on modern distributions instead of a… (#12676)
* Use annotations to give feedback on modern distributions instead of a pull request comment * Use encoded newlines * Use correct lines * Add dependency * lf * Fix distribution name * Various fixes * Cleanup diff * Cleanup diff * Add test flag * Undo test change
Blue committed
Mar 12, 2025 at 16:07 UTC
1e8588fdeaa4493cd43f62bc2e456bb46f6cf2ea
3 files changed
+101
-98
.github/workflows/modern-distributions.yml
+1
-5
@@ -8,8 +8,6 @@ jobs:
8
check:
9
name: Validate tar based distributions changes
10
runs-on: ubuntu-latest
11
- permissions:
12
- pull-requests: write
11
steps:
12
- name: Checkout repo
13
uses: actions/checkout@v4
@@ -26,7 +24,5 @@ jobs:
24
--repo-path . \
25
--compare-with-branch 'origin/${{ github.base_ref }}' \
26
--manifest distributions/DistributionInfo.json \
29
- --github-token '${{ secrets.GITHUB_TOKEN }}' \
30
- --github-pr '${{ github.event.pull_request.number }}' \
31
- --github-commit '${{ github.sha }}'
27
+
28
shell: bash
distributions/requirements.txt
+2
-1
@@ -1,4 +1,5 @@
1
python-magic==0.4.27
2
click==8.1.3
3
GitPython==3.1.41
4
-PyGithub==2.5.0
\ No newline at end of file
4
+PyGithub==2.5.0
5
+json-cfg==0.4.2
\ No newline at end of file
distributions/validate-modern.py
+98
-92
@@ -1,5 +1,6 @@
1
import click
2
-import json
2
+import jsoncfg
3
+from jsoncfg.config_classes import ConfigJSONObject, ConfigJSONArray, ConfigJSONScalar
4
import requests
5
import tempfile
6
import hashlib
@@ -31,9 +32,27 @@ DISCOURAGED_SYSTEM_UNITS = ['systemd-resolved.service',
32
'NetworkManager.service',
33
'networking.service']
34
34
-errors = []
35
-warnings = []
35
+errors = {}
36
+warnings = {}
37
38
+def subset(inner, outer) -> bool:
39
+ for key, value in inner:
40
+ if key not in outer:
41
+ return True
42
+
43
+ if not node_equals(value, outer[key]):
44
+ return False
45
+
46
+ return True
47
+
48
+def node_equals(left, right):
49
+ if isinstance(left, ConfigJSONScalar):
50
+ return left() == right()
51
+
52
+ elif isinstance(left, ConfigJSONArray):
53
+ return len(left) == len(right) and all(node_equals(l, r) for l, r in zip(left, right))
54
+ else:
55
+ return subset(left, right) and subset(right, left)
56
57
@click.command()
58
@click.option('--manifest', default=None)
@@ -41,11 +60,8 @@ warnings = []
60
@click.option('--compare-with-branch')
61
@click.option('--repo-path', '..')
62
@click.option('--arm64', is_flag=True)
44
-@click.option('--github-token', default=None)
45
-@click.option('--github-pr', default=None, type=int)
46
-@click.option('--github-commit', default=None)
63
@click.option('--debug', is_flag=True)
48
-def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm64: bool, github_token: str, github_pr: str, github_commit: str, debug: bool):
64
+def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm64: bool, debug: bool):
65
try:
66
if tar is not None:
67
with open(tar, 'rb') as fd:
@@ -54,69 +70,64 @@ def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm6
70
if manifest is None:
71
raise RuntimeError('Either --tar or --manifest is required')
72
57
- with open(manifest) as fd:
58
- manifest_content = json.loads(fd.read())
73
+ manifest_content = jsoncfg.load_config(manifest)
74
75
baseline_manifest = None
76
if compare_with_branch is not None:
77
repo = git.Repo(repo_path)
78
baseline_json = repo.commit(compare_with_branch).tree / 'distributions/DistributionInfo.json'
64
- baseline_manifest = json.load(baseline_json.data_stream).get('ModernDistributions', {})
79
+ baseline_manifest = jsoncfg.loads_config(baseline_json.data_stream.read().decode())['ModernDistributions']
80
66
- for flavor, versions in manifest_content["ModernDistributions"].items():
67
- baseline_flavor = baseline_manifest.get(flavor, None) if baseline_manifest else None
81
+ for flavor, versions in manifest_content["ModernDistributions"]:
82
+ baseline_flavor = baseline_manifest[flavor] if baseline_manifest and flavor in baseline_manifest else None
83
84
for e in versions:
70
- name = e.get('Name', None)
85
+ name = e['Name']() if 'Name' in e else None
86
87
if name is None:
73
- error(flavor, None, 'Found nameless distribution')
88
+ error(flavor, 'Found nameless distribution')
89
continue
90
91
if baseline_flavor is not None:
77
- baseline_version = next((entry for entry in baseline_flavor if entry['Name'] == name), None)
92
+ baseline_version = next((entry for entry in baseline_flavor if entry['Name']() == name), None)
93
if baseline_version is None:
94
click.secho(f'Found new entry for flavor "{flavor}": {name}', fg='green', bold=True)
80
- elif baseline_version != e:
95
+ elif not node_equals(baseline_version, e):
96
click.secho(f'Found changed entry for flavor "{flavor}": {name}', fg='green', bold=True)
97
else:
98
click.secho(f'Distribution entry "{flavor}/{name}" is unchanged, skipping')
99
continue
100
86
- click.secho(f'Reading information for distribution: {e["Name"]}', bold=True)
101
+ click.secho(f'Reading information for distribution: {name}', bold=True)
102
if 'FriendlyName' not in e:
88
- error(flavor, name, 'Manifest entry is missing a "FriendlyName" entry')
103
+ error(e, 'Manifest entry is missing a "FriendlyName" entry')
104
105
if not name.startswith(flavor):
91
- error(flavor, name, f'Name should start with "{flavor}"')
106
+ error(e, f'Name should start with "{flavor}"')
107
108
url_found = False
109
110
if 'Amd64Url' in e:
96
- read_url(flavor, name, e['Amd64Url'], X64_ELF_MAGIC)
111
+ read_url(e['Amd64Url'], X64_ELF_MAGIC)
112
url_found = True
113
114
if 'Arm64Url' in e:
100
- read_url(flavor, name, e['Arm64Url'], ARM64_ELF_MAGIC)
115
+ read_url(e['Arm64Url'], ARM64_ELF_MAGIC)
116
url_found = True
117
118
if not url_found:
104
- error(flavor, name, 'No URL found')
119
+ error(flavor, 'No URL found')
120
121
expectedKeys = ['Name', 'FriendlyName', 'Default', 'Amd64Url', 'Arm64Url']
107
- for key in e.keys():
122
+ for key, value in e:
123
if key not in expectedKeys:
109
- error(flavor, name, 'Unexpected key: "{key}"')
110
-
124
+ error(e, f'Unexpected key: "{key}"')
125
112
- default_entries = sum(1 for e in versions if e.get('Default', False))
126
+ default_entries = sum(1 for e in versions if 'Default' in e and e['Default']())
127
if default_entries != 1:
114
- error(flavor, None, 'Found no default distribution' if default_entries == 0 else 'Found multiple default distributions')
128
+ error(e, 'Found no default distribution' if default_entries == 0 else 'Found multiple default distributions')
129
116
- if github_pr is not None:
117
- assert github_token is not None and github_commit is not None and manifest is not None
118
-
119
- report_status_on_pr(github_pr, github_token, github_commit, manifest)
130
+ report_status_on_pr(manifest)
131
132
except:
133
if debug:
@@ -127,30 +138,22 @@ def main(manifest: str, tar: str, compare_with_branch: str, repo_path: str, arm6
138
else:
139
raise
140
130
-def report_status_on_pr(pr: int, github_token: str, github_commit: str, manifest: str):
131
- github = Github(github_token)
132
- repo = github.get_repo('microsoft/WSL')
133
-
141
+def report_status_on_pr(manifest: str):
142
def format_list(entries: list) -> str:
135
- output = '\n'
143
+ if len(entries) == 1:
144
+ return entries[0]
145
146
+ output = ''
147
for e in entries:
148
output += f'\n* {e}'
149
140
- return output + '\n'
150
+ return output
151
142
- body = 'Thank you for your contribution to WSL.\n'
143
- if errors:
144
- body += f'**The following fatal errors have been found in this pull request:** {format_list(errors)}\n'
145
- else:
146
- body += 'No fatal errors have been found.\n'
152
+ for line, text in errors.items():
153
+ print(f'::error file={manifest},line={line}::Error: {format_list(text).replace('\n', '%0A')}')
154
148
- if warnings:
149
- body += f'**The following suggestions have been found in this pull request:** {format_list(warnings)}\n'
150
- else:
151
- body += 'No suggestions have been found.\n'
152
-
153
- repo.get_pull(pr).create_review(body=body, commit=repo.get_commit(github_commit))
155
+ for line, text in warnings.items():
156
+ print(f'::warning file={manifest},line={line}::Warning: {format_list(text).replace('\n', '%0A')}')
157
158
159
def read_config_keys(config: configparser.ConfigParser) -> dict:
@@ -162,17 +165,17 @@ def read_config_keys(config: configparser.ConfigParser) -> dict:
165
166
return keys
167
165
-def read_passwd(flavor: str, name: str, default_uid: int, fd):
168
+def read_passwd(node, default_uid: int, fd):
169
def read_passwd_line(line: str):
170
fields = line.split(':')
171
172
if len(fields) != 7:
170
- error(flavor, name, f'Invalid passwd entry: {line}')
173
+ error(node, f'Invalid passwd entry: {line}')
174
return None, None
175
try:
176
uid = int(fields[2])
177
except ValueError:
175
- error(flavor, name, f'Invalid passwd entry: {line}')
178
+ error(node, f'Invalid passwd entry: {line}')
179
return None, None
180
181
return uid, fields
@@ -183,20 +186,20 @@ def read_passwd(flavor: str, name: str, default_uid: int, fd):
186
uid, fields = read_passwd_line(line.decode())
187
188
if uid in entries:
186
- error(flavor, name, f'found duplicated uid in /etc/passw: {uid}')
189
+ error(node, f'found duplicated uid in /etc/passw: {uid}')
190
else:
191
entries[uid] = fields
192
193
if 0 not in entries:
194
error(flavor, name, f'No root (uid=0) found in /etc/passwd')
195
elif entries[0][0] != 'root':
193
- error(flavor, name, f'/etc/passwd has a uid=0, but it is not root: {entries[0][0]}')
196
+ error(node, f'/etc/passwd has a uid=0, but it is not root: {entries[0][0]}')
197
198
if default_uid is not None and default_uid in entries:
196
- warning(flavor, name, f'/etc/passwd already has an entry for default uid: {entries[default_uid]}')
199
+ warning(node, f'/etc/passwd already has an entry for default uid: {entries[default_uid]}')
200
201
# This logic isn't perfect at listing all boot units, but parsing all of systemd configuration would be too complex.
199
-def read_systemd_enabled_units(flavor: str, name: str, tar) -> dict:
202
+def read_systemd_enabled_units(node, tar) -> dict:
203
config_dirs = ['/usr/local/lib/systemd/system', '/usr/lib/systemd/system', '/etc/systemd/system']
204
205
all_files = tar.getnames()
@@ -317,7 +320,7 @@ def get_tar_file(tar, path: str, follow_symlink=False, symlink_depth=10):
320
321
return None, None
322
320
-def read_tar(flavor: str, name: str, file, elf_magic: str):
323
+def read_tar(node, file, elf_magic: str):
324
with tarfile.open(fileobj=file) as tar:
325
326
def validate_mode(path: str, mode, uid, gid, max_size = None, optional = False, follow_symlink = False, magic = None, parse_method = None):
@@ -330,16 +333,16 @@ def read_tar(flavor: str, name: str, file, elf_magic: str):
333
334
permissions = oct(info.mode)
335
if permissions not in mode:
333
- warning(flavor, name, f'file: "{path}" has unexpected mode: {permissions} (expected: {mode})')
336
+ warning(node, f'file: "{path}" has unexpected mode: {permissions} (expected: {mode})')
337
338
if info.uid != uid:
336
- warning(flavor, name, f'file: "{path}" has unexpected uid: {info.uid} (expected: {uid})')
339
+ warning(node, f'file: "{path}" has unexpected uid: {info.uid} (expected: {uid})')
340
341
if gid is not None and info.gid != gid:
339
- warning(flavor, name, f'file: "{path}" has unexpected gid: {info.gid} (expected: {gid})')
342
+ warning(node, f'file: "{path}" has unexpected gid: {info.gid} (expected: {gid})')
343
344
if max_size is not None and info.size > max_size:
342
- error(flavor, name, f'file: "{path}" is too big (info.size), max: {max_size}')
345
+ error(node, f'file: "{path}" is too big (info.size), max: {max_size}')
346
347
if magic is not None or parse_method is not None:
348
content = tar.extractfile(real_path)
@@ -352,14 +355,14 @@ def read_tar(flavor: str, name: str, file, elf_magic: str):
355
buffer = content.read(256)
356
file_magic = MAGIC.from_buffer(buffer)
357
if not magic.match(file_magic):
355
- error(flavor, name, f'file: "{path}" has unexpected magic type: {file_magic} (expected: {magic})')
358
+ error(node, f'file: "{path}" has unexpected magic type: {file_magic} (expected: {magic})')
359
360
return True
361
362
def validate_config(path: str, valid_keys: list):
363
_, path = get_tar_file(tar, path, follow_symlink=True)
364
if path is None:
362
- error(flavor, name, f'File "{file}" not found in tar')
365
+ error(node, f'File "{file}" not found in tar')
366
return None
367
368
content = tar.extractfile(path)
@@ -370,7 +373,7 @@ def read_tar(flavor: str, name: str, file, elf_magic: str):
373
374
unexpected_keys = [e for e in keys if e.lower() not in valid_keys]
375
if unexpected_keys:
373
- error(flavor, name, f'Found unexpected_keys in "{path}": {unexpected_keys}')
376
+ error(node, f'Found unexpected_keys in "{path}": {unexpected_keys}')
377
else:
378
click.secho(f'Found valid keys in "{path}": {list(keys.keys())}')
379
@@ -384,11 +387,11 @@ def read_tar(flavor: str, name: str, file, elf_magic: str):
387
validate_mode(oobe_command, [oct(0o775), oct(0o755)], 0, 0)
388
389
if not oobe_command.startswith(USR_LIB_WSL):
387
- warning(flavor, name, f'value for oobe.command is not under {USR_LIB_WSL}: "{oobe_command}"')
390
+ warning(node, f'value for oobe.command is not under {USR_LIB_WSL}: "{oobe_command}"')
391
392
if defaultUid := config.get('oobe.defaultuid', None):
393
if defaultUid != '1000':
391
- warning(flavor, name, f'Default UID is not 1000. Found: {defaultUid}')
394
+ warning(node, f'Default UID is not 1000. Found: {defaultUid}')
395
396
defaultUid = int(defaultUid)
397
@@ -396,37 +399,39 @@ def read_tar(flavor: str, name: str, file, elf_magic: str):
399
validate_mode(shortcut_icon, [oct(0o664), oct(0o644)], 0, 0, 1024 * 1024)
400
401
if not shortcut_icon.startswith(USR_LIB_WSL):
399
- warning(flavor, name, f'value for shortcut.icon is not under {USR_LIB_WSL}: "{shortcut_icon}"')
402
+ warning(node, f'value for shortcut.icon is not under {USR_LIB_WSL}: "{shortcut_icon}"')
403
404
if terminal_profile := config.get('windowsterminal.profileTemplate', None):
405
validate_mode(terminal_profile, [oct(0o660), oct(0o640)], 0, 0, 1024 * 1024)
406
407
if not terminal_profile.startswith(USR_LIB_WSL):
405
- warning(flavor, name, f'value for windowsterminal.profileTemplate is not under {USR_LIB_WSL}: "{terminal_profile}"')
408
+ warning(node, f'value for windowsterminal.profileTemplate is not under {USR_LIB_WSL}: "{terminal_profile}"')
409
410
if validate_mode('/etc/wsl.conf', [oct(0o664), oct(0o644)], 0, 0, optional=True):
411
config = validate_config('/etc/wsl.conf', ['boot.systemd'])
412
if config.get('boot.systemd', False):
413
validate_mode('/sbin/init', [oct(0o775), oct(0o755)], 0, 0, magic=elf_magic, follow_symlink=True)
414
412
- validate_mode('/etc/passwd', [oct(0o664), oct(0o644)], 0, 0, parse_method = lambda fd: read_passwd(flavor, name, defaultUid, fd))
415
+ validate_mode('/etc/passwd', [oct(0o664), oct(0o644)], 0, 0, parse_method = lambda fd: read_passwd(node, defaultUid, fd))
416
validate_mode('/etc/shadow', [oct(0o640), oct(0o600)], 0, None)
417
validate_mode('/bin/bash', [oct(0o755), oct(0o775)], 0, 0, magic=elf_magic, follow_symlink=True)
418
validate_mode('/bin/sh', [oct(0o755), oct(0o775)], 0, 0, magic=elf_magic, follow_symlink=True)
419
417
- enabled_systemd_units = read_systemd_enabled_units(flavor, name, tar)
420
+ enabled_systemd_units = read_systemd_enabled_units(node, tar)
421
for unit, path in enabled_systemd_units.items():
422
if unit in DISCOURAGED_SYSTEM_UNITS:
420
- warning(flavor, name, f'Found discouraged system unit: {path}')
423
+ warning(node, f'Found discouraged system unit: {path}')
424
422
-def read_url(flavor: str, name: str, url: dict, elf_magic):
425
+def read_url(url: dict, elf_magic):
426
hash = hashlib.sha256()
424
- if not url['Url'].endswith('.wsl'):
425
- warning(flavor, name, f'Url does not point to a .wsl file: {url["Url"]}')
427
+ address = url['Url']()
428
+
429
+ if not address.endswith('.wsl'):
430
+ warning(url, f'Url does not point to a .wsl file: {address}')
431
432
tar_format = None
428
- if url['Url'].startswith('file://'):
429
- with open(url['Url'].replace('file:///', '').replace('file://', ''), 'rb') as fd:
433
+ if address.startswith('file://'):
434
+ with open(address.replace('file:///', '').replace('file://', ''), 'rb') as fd:
435
while True:
436
e = fd.read(4096 * 4096 * 10)
437
if not e:
@@ -438,9 +443,9 @@ def read_url(flavor: str, name: str, url: dict, elf_magic):
443
tar_format = MAGIC.from_buffer(e)
444
445
fd.seek(0, 0)
441
- read_tar(flavor, name, fd, elf_magic)
446
+ read_tar(url, fd, elf_magic)
447
else:
443
- with requests.get(url['Url'], stream=True) as response:
448
+ with requests.get(address, stream=True) as response:
449
response.raise_for_status()
450
451
with tempfile.NamedTemporaryFile() as file:
@@ -452,42 +457,43 @@ def read_url(flavor: str, name: str, url: dict, elf_magic):
457
tar_format = MAGIC.from_buffer(e)
458
459
file.seek(0, 0)
455
- read_tar(flavor, name, file, elf_magic)
460
+ read_tar(url, file, elf_magic)
461
462
458
- expected_sha = url.get('Sha256', None)
463
+ expected_sha = url['Sha256']() if 'Sha256' in url else None
464
if expected_sha is None:
460
- error(flavor, name, 'URL is missing "Sha256"')
465
+ error(url, 'URL is missing "Sha256"')
466
else:
467
if expected_sha.startswith('0x'):
468
expected_sha = expected_sha[2:]
469
470
sha = hash.digest()
471
if bytes.fromhex(expected_sha) != sha:
467
- error(flavor, name, f'URL {url["Url"]} Sha256 does not match. Expected: {expected_sha}, actual: {hash.hexdigest()}')
472
+ error(url, f'URL {address} Sha256 does not match. Expected: {expected_sha}, actual: {hash.hexdigest()}')
473
else:
469
- click.secho(f'Hash for {url["Url"]} matches ({expected_sha})', fg='green')
474
+ click.secho(f'Hash for {address} matches ({expected_sha})', fg='green')
475
476
known_format = next((value for key, value in KNOWN_TAR_FORMATS.items() if re.match(key, tar_format)), None)
477
if known_format is None:
473
- error(flavor, name, f'Unknown tar format: {tar_format}')
478
+ error(url, f'Unknown tar format: {tar_format}')
479
elif not known_format:
475
- warning(flavor, name, f'Tar format not supported by WSL1: {tar_format}')
480
+ warning(url, f'Tar format not supported by WSL1: {tar_format}')
481
477
-def error(flavor: str, distribution: str, message: str):
482
+def error(node, message: str):
483
global errors
484
480
- message = f'{flavor}/{distribution}: {message}'
481
- click.secho(f'Error: {message}', fg='red')
485
+ line = jsoncfg.node_location(node).line
486
+ click.secho(f'Error on line {line}: {message}', fg='red')
487
483
- errors.append(message)
488
+ errors[line] = errors.get(line, []) + [message]
489
485
-def warning(flavor: str, distribution: str, message: str):
490
+def warning(node, message: str):
491
global warnings
492
488
- message = f'{flavor}/{distribution}: {message}'
489
- click.secho(f'Warning: {message}', fg='yellow')
493
+ line = jsoncfg.node_location(node).line
494
+ click.secho(f'Warning on line {line}: {message}', fg='yellow')
495
+
496
+ warnings[line] = warnings.get(line, []) + [message]
497
491
- warnings.append(message)
498
if __name__ == "__main__":
499
main()
\ No newline at end of file