Run repo checks entirely with Github Actions #8050
Comments
|
It had to be refactored a bit because running workflows from PRs created from forked repositories (so basically the usual case in this repo) didn't work correctly. In those cases, GitHub doesn't pass the access token to the workflow so the review code was unable to post review comments. The solution to that is using a pull_request_target event rather than Coincidentally it also makes it easier to set up. There are now two actions:
An example of how to trigger those from the workflow is at https://github.com/sublimelsp/st-package-reviewer-action |
|
It appears some efforts were made to refactor package reviewer, so it can run as Github Action. Would it make sense to move the action to a more common organization, which could probably also be the target to collect the other parts/repos at some point? We could organize everything under SublimeText or packagecontrol org. Thoughts? |
|
I agree the code of those actions should be moved. Just a small note that I've recalled when looking at the code yesterday... There is this part in the reviewer (https://github.com/sublimelsp/st-package-reviewer-action/blob/e98168e07368ac1cb0f4d0c790b273195d9b8f89/lib/run_repo_tests.py#L175-L181) that I've disabled because apparently the original reviewer code on github is not up to date with what ST repo runs. |
packagecontrol org is definitely my proposal, next to the package reviewer itself. |
|
I think so, too, especially with the idea in mind to also move all the other package_control related repos over there. So everything would be at one place. @rchl: Would you initiate repo transfer? @FichteFoll: Could you add us as maintainer? |
|
I can do that. |
|
That's why I asked Fichte to add us as member/maintainer. He seems to be the org admin there. I guess, those actions need some updates to support PC 4.0.0 schemes in future as well. |
What are you referring to now? I meant that probably only wbond has the missing code in his local repo (or maybe on the PC server). So I don't think Fichte can help with that. |
|
Well, maybe I am missing something, but it appears to me st_package_reviewer is mainly authored and mainteined by Fichte on github.com/packagecontrol org and st-package-reviewer-action installing it via pip to make use of it. The action also vendors latest package_control 3.4.1 So, which local changes do you expect to be missing? |
|
It's been a while since I've looked into it but I believe it's about those lines that I've commented out: if checker == CheckMessages:
for release_source in spec['releases']:
if isinstance(release_source.get('tags'), str):
checker_obj.add_prefix(release_source.get('tags'))
elif checker == CheckHasSublimeSyntax:
checker_obj.set_selector(info['releases'][0]['sublime_text'])The |
Done (for the org). (I thought @deathaxe was already an admin there tbh.) Regarding the "missing code": That's some code that Will added for the packagecontrol.io site specifically. You should be able to find it in the repo. I think I reviewed/monitored that at some point, but it's been many years since then and I don't remember what exactly he added outside of submitting the review comment via the bot account. Keep in mind that the version of st_package_reviewer inside the packagecontrol.io repo is based on an older version and does not have the newer AST checks, for example. I'm also unsure what the current state of the st_package_reviwer repo is, to be honest, because I know I had plans for it (and some actual TODO items, like re-considering many of the AST checks that I haven't really tested on packages in the wild and accepting a "min version" parameter based on the sublime_text version selector used in the channel so that some checks are automatically disabled or ignored, like the sublime-syntax vs tmLanguage check). |
|
The state is a bit confusing indeed. I've compared those two repos (the Left: /packagecontrol.io/app/lib/st_package_reviewer --- check/file/ast/__init__.py 2020-09-19 21:56:43.000000000
+++ check/file/ast/__init__.py 2023-10-12 21:20:56.000000000
@@ -1,11 +1,11 @@
import functools
import ast
from pathlib import Path
-from st_package_reviewer.check.file import FileChecker
-from st_package_reviewer.check import find_all
+from ....check.file import FileChecker
+from ....check import find_all
__all__ = ('AstChecker', 'get_checkers')
class AstChecker(FileChecker, ast.NodeVisitor):
"""Groups checks for python source code."""
@@ -34,14 +34,14 @@
with path.open("r") as f:
try:
# Cast path to string for <py36
the_ast = ast.parse(f.read(), str(path))
except SyntaxError as e:
- self.fail("Unable to parse Python file (column {})".format(e.offset + 1),
- exception=e)
+ with self.context("Line: {}".format(e.lineno)):
+ self.fail("Unable to parse Python file", exception=e)
else:
self._ast_cache[path] = the_ast
return the_ast
def node_context(self, node):
return self.context("Line: {}, Column: {}".format(node.lineno, node.col_offset + 1))
--- check/file/ast/check_initialized_api.py 2020-09-19 21:56:43.000000000
+++ check/file/ast/check_initialized_api.py 2023-10-12 21:19:54.000000000
@@ -19,13 +19,12 @@
if base.id in interesting:
return True
return False
class CheckInitializedApiUsage(AstChecker):
-
"""Check for function calls of the sublime module in unsafe places.
Notably, these are:
- the global module scope
- __init__ methods of event listeners
@@ -81,13 +80,13 @@
# All 'remaining' calls of `sublime` attributes that are not safe
# must be in places where the API may not have been initialized.
try:
attr = node.func.attr
id_ = node.func.value.id
- except AttributeError as e:
+ except AttributeError:
return
else:
if id_ == "sublime" and attr not in ("platform", "arch", "version", "channel"):
with self.node_context(node):
self.fail("Calling unsafe method {!r} of sublime module"
" when API may not have been initialized".format(attr))
--- check/file/ast/check_no_modify_sys_path.py 2020-09-19 21:56:43.000000000
+++ check/file/ast/check_no_modify_sys_path.py 2023-10-12 21:19:56.000000000
@@ -16,8 +16,8 @@
if (
func.attr in {'append', 'insert'}
and func.value.attr == 'path'
and func.value.value.id == 'sys'
):
self._warn_about_modify_sys_path(node)
- except Exception as e:
+ except Exception:
return
--- check/file/ast/check_os_system_calls.py 2020-09-19 21:56:43.000000000
+++ check/file/ast/check_os_system_calls.py 2023-10-12 21:19:57.000000000
@@ -10,10 +10,10 @@
"Also make sure you thought about the platform key in your pull request.")
def visit_Call(self, node):
try:
attr = node.func.attr
id = node.func.value.id
- except Exception as e:
+ except Exception:
return
if id == "os" and attr == "system":
self._warn_about_os_system(node)
--- check/file/ast/check_platform_usage.py 2020-09-19 21:56:43.000000000
+++ check/file/ast/check_platform_usage.py 2023-10-12 21:19:59.000000000
@@ -27,10 +27,10 @@
self._warn_platform_module_usage(node)
def visit_Call(self, node):
try:
attr = node.func.attr
id_ = node.func.value.id
- except Exception as e:
+ except Exception:
return
if id_ == "sublime" and attr in ("platform", "arch"):
self._warn_sublime_platform_usage(node)
--- check/file/check_keymaps.py 2020-09-19 21:56:43.000000000
+++ check/file/check_keymaps.py 2023-10-12 21:20:01.000000000
@@ -83,12 +83,18 @@
supplementary_keys = keys - allowed_keys
if supplementary_keys:
self.warn("Binding defines supplementary keys {}".format(supplementary_keys))
if 'keys' in binding:
+ if binding['keys'] == ["<character>"]:
+ if not binding.get('context'):
+ self.fail("'<character>' bindings must have a 'context'")
+ idx_to_del.add(i)
+ continue
+
try:
norm_chords = k_map._verify_and_normalize_chords(binding['keys'])
except KeyMappingError as e:
self.fail(e.args[0])
idx_to_del.add(i)
else:
@@ -108,13 +114,12 @@
class KeyMapping:
_def_maps = None
@classmethod
def default_maps(cls):
- # type: Dict[str, KeyMapping]
if not cls._def_maps:
cls._def_maps = {plat: cls(DATA_PATH / fname)
for plat, fname in zip(PLATFORMS, PLATFORM_FILENAMES)}
# Verify and normalize default maps
for k_map in cls._def_maps.values():
k_map._verify()
@@ -142,41 +147,48 @@
def _verify(self):
for binding in self.data:
binding['keys'] = self._verify_and_normalize_chords(binding['keys'])
@classmethod
def _verify_and_normalize_chords(cls, chords):
- modifiers = ("ctrl", "super", "alt", "shift", "primary")
-
if not chords or not isinstance(chords, list):
raise KeyMappingError("'keys' key is empty or not a list")
norm_chords = []
for key_chord in chords:
if len(key_chord) == 1:
# Any single character key is valid (representing a symbol)
+ norm_chords.append(key_chord)
+ continue
+
+ elif key_chord in cls.MODIFIERS:
+ # Legal since ST4
+ if len(chords) == 1:
+ # But we disallow
+ # TODO report minimum version
+ raise KeyMappingError("Single-chord modifier bindings are disallowed")
norm_chords.append(key_chord)
continue
chord_parts = []
while True:
key, plus, key_chord = key_chord.partition("+")
if not key_chord: # we're at the end
if plus: # a chord with '+' as key
key = plus
if not cls._key_is_valid(key):
raise KeyMappingError("Invalid key '{}'".format(key))
- chord_parts.sort(key=modifiers.index)
+ chord_parts.sort(key=cls.MODIFIERS.index)
chord_parts.append(key)
break
if key == "option":
key = "alt"
elif key == "command":
key = "super"
# TODO "primary"
- if key not in modifiers:
+ if key not in cls.MODIFIERS:
raise KeyMappingError("Invalid modifier key '{}'".format(key))
chord_parts.append(key)
norm_chords.append("+".join(chord_parts))
@@ -211,6 +223,8 @@
"clear", "sysreq",
# these have single-character equivalents
# TODO resolve these aliases
"plus", "minus", "equals", "forward_slash", "backquote",
# Note: this list is incomplete and sourced from the default bindings
}
+
+ MODIFIERS = ("ctrl", "super", "alt", "altgr", "shift", "primary")
--- check/file/check_license.py 2020-09-19 21:56:43.000000000
+++ check/file/check_license.py 2023-10-12 21:18:51.000000000
@@ -5,12 +5,12 @@
class CheckLicense(FileChecker):
def check(self):
has_license = any(
True for p in self.base_path.iterdir()
- if re.search(r'(?i)^license', p.name)
+ if re.search(r'(?i)^(un)?license', p.name)
)
if not has_license:
self.warn("The package does not contain a top-level LICENSE file."
" A license helps users to contribute to the package.")
--- check/file/check_messages.py 2020-09-19 21:56:43.000000000
+++ check/file/check_messages.py 2023-10-12 21:20:56.000000000
@@ -1,27 +1,16 @@
import json
-import re
from ...lib.semver import SemVer
from . import FileChecker
class CheckMessages(FileChecker):
- prefixes = None
-
- def add_prefix(self, prefix):
- if self.prefixes is None:
- self.prefixes = {'v'}
- self.prefixes.add(prefix)
-
def check(self):
- if self.prefixes is None:
- self.prefixes = {'v'}
-
msg_path = self.sub_path("messages.json")
folder_exists = self.sub_path("messages").is_dir()
file_exists = msg_path.is_file()
if not (folder_exists or file_exists):
return
@@ -35,20 +24,19 @@
try:
data = json.load(f)
except ValueError as e:
self.fail("unable to load `messages.json`", exception=e)
return
- prefix_regex = '^(' + '|'.join(list(self.prefixes)) + ')'
for key, rel_path in data.items():
if key == "install":
pass
- elif SemVer.valid(re.sub(prefix_regex, '', key)):
+ elif SemVer.valid(key):
pass
else:
self.fail("Key {!r} is not 'install' or a valid semantic version"
.format(key))
messsage_path = self.sub_path(rel_path)
if not messsage_path.is_file():
self.fail("File '{}', as specified by key {!r}, does not exist"
.format(rel_path, key))
--- check/file/check_resource_file_validity.py 2020-09-19 21:56:43.000000000
+++ check/file/check_resource_file_validity.py 2023-10-12 21:20:56.000000000
@@ -1,7 +1,6 @@
-import sys
import plistlib
import xml.etree.ElementTree as ET
from xml.parsers.expat import ExpatError
from . import FileChecker
from ...lib import jsonc
@@ -47,16 +46,13 @@
}
for file_path in self.globs(*plist_file_globs):
with self.file_context(file_path):
with file_path.open('rb') as f:
try:
- if sys.version_info < (3, 4):
- plistlib.readPlist(f)
- else:
- plistlib.load(f)
+ plistlib.load(f)
except (ValueError, ExpatError) as e:
self.fail("Invalid Plist", exception=e)
class CheckXmlFiles(FileChecker):
--- check/file/check_resource_files.py 2020-09-19 21:56:43.000000000
+++ check/file/check_resource_files.py 2023-10-12 21:20:56.000000000
@@ -1,8 +1,7 @@
import logging
-import re
from . import FileChecker
l = logging.getLogger(__name__)
@@ -23,17 +22,20 @@
.format(len(python_files_in_package)))
class CheckHasResourceFiles(FileChecker):
def check(self):
+ # Files with a hidden extension are excluded,
+ # as they serve no purpose without another file using them
+ # (e.g. a plugin).
resource_file_globs = {
"*.py",
"**/*.sublime-build",
"**/*.sublime-color-scheme",
- "**/*.hidden-color-scheme",
+ # "**/*.hidden-color-scheme",
"**/*.sublime-commands",
"**/*.sublime-completions",
"**/*.sublime-keymap",
"**/*.sublime-macro", # almost useless without other files
"**/*.sublime-menu",
"**/*.sublime-mousemap",
@@ -42,71 +44,30 @@
"**/*.sublime-syntax",
"**/*.sublime-theme",
"**/*.tmLanguage",
"**/*.tmPreferences",
"**/*.tmSnippet",
"**/*.tmTheme",
- "**/*.hidden-tmTheme",
+ # "**/*.hidden-tmTheme",
# hunspell dictionaries
"**/*.aff",
"**/*.dic",
}
has_resource_files = any(self.glob(ptrn) for ptrn in resource_file_globs)
if not has_resource_files:
self.fail("The package does not define any file that interfaces with Sublime Text")
class CheckHasSublimeSyntax(FileChecker):
- selector = None
-
- def set_selector(self, selector):
- self.selector = selector
-
- def st_build_match(self, ver):
- min_version = float("-inf")
- max_version = float("inf")
-
- if self.selector == '*':
- return True
-
- gt_match = re.match('>(\d+)$', self.selector)
- ge_match = re.match('>=(\d+)$', self.selector)
- lt_match = re.match('<(\d+)$', self.selector)
- le_match = re.match('<=(\d+)$', self.selector)
- range_match = re.match('(\d+) - (\d+)$', self.selector)
-
- if gt_match:
- min_version = int(gt_match.group(1)) + 1
- elif ge_match:
- min_version = int(ge_match.group(1))
- elif lt_match:
- max_version = int(lt_match.group(1)) - 1
- elif le_match:
- max_version = int(le_match.group(1))
- elif range_match:
- min_version = int(range_match.group(1))
- max_version = int(range_match.group(2))
- else:
- return None
-
- if min_version > ver:
- return False
- if max_version < ver:
- return False
-
- return True
-
def check(self):
syntax_files = self.glob("**/*.sublime-syntax")
for path in syntax_files:
if (
not path.with_suffix(".tmLanguage").is_file()
and not path.with_suffix(".hidden-tmLanguage").is_file()
):
- if not self.st_build_match(3091) and not self.st_build_match(2221):
- continue
with self.file_context(path):
self.warn("'.sublime-syntax' support has been added in build 3092 and there "
"is no '.tmLanguage' fallback file")
--- check/__init__.py 2020-09-19 21:56:43.000000000
+++ check/__init__.py 2023-10-12 21:20:56.000000000
@@ -76,20 +76,13 @@
rel_path = checker_file.relative_to(path)
if rel_path.name == "__init__.py":
l.debug("Skipping %s", rel_path)
continue
l.debug("Loading %s...", rel_path)
- # The Python 3.3 shim for pathlib doesn't support: .with_suffix('')
- if sys.version_info < (3, 4):
- rel_path_no_suffix = str(rel_path)
- if rel_path_no_suffix.endswith('.py'):
- rel_path_no_suffix = rel_path_no_suffix[:-3]
- else:
- rel_path_no_suffix = str(rel_path.with_suffix(''))
- relative_module_segments = rel_path_no_suffix.split(os.sep)
+ relative_module_segments = str(rel_path.with_suffix('')).split(os.sep)
module_path = "." + ".".join(relative_module_segments)
module = importlib.import_module(module_path, package)
for thing in module.__dict__.values():
if not isinstance(thing, type): # not a class
continue
---
+++ lib/__init__.py 2023-10-12 21:20:56.000000000
@@ -0,0 +1 @@
+"""Package for third-party modules that aren't bundled on pypi."""Besides that, the https://github.com/packagecontrol/st_package_reviewer/tree/master/st_package_reviewer/check/repo contains a The https://github.com/packagecontrol/st_package_reviewer seems to have mostly newer changes but a notable thing that is missing in it are I think I mostly know how to handle it so I could pick and choose the correct changes myself in the action's repo but it would also be nice if https://github.com/wbond/packagecontrol.io repo would get updated with newer changes. |
|
I've transferred both repos now:
Two notes though:
|
Those have been added specifically for the automatic review PR review flow from the pcc PR whereas originally
The bundled version of the reviwer in the packagecontrol.io repo should be abandoned. It stems from an era before GitHub Actions where a thing, but since we have those now there is absolutely no need to bother a central service with running the tests, which is exactly what this ticket is about. If it has been implemented, the manual endpoints for the packagecontrol.io site to trigger a PR review is redundant and should also be removed, because it's actually somewhat of a DoS entrypoint. |
So where do we stand now? As for the And for the |
|
I had a look at The action is designed to be used for package_control_channel repo at the moment. I'd find it useful to at least provide input parameters for channel and repository files to verify. Most included repositories are named "packages.json", which the tests don't work for as they expect "repositories.json". I had a look at syntax-test-action how to achieve that, but I haven't got into closer touch with that sort of stuff. Appears some environment variables are needed to pass arguments. The only thing I have so far is an adjusted action.yml: name: Sublime Text Schema Reviewer
description: GitHub Action for reviewing package package control schema
inputs:
channel:
description: Channel file to check
required: false
default: channel.json
channel_test_repositories:
description: Check channel's repositories
required: false
default: false
repository:
description: Repository file to check
required: false
default: repository.json
runs:
using: 'composite'
steps:
- name: Run repository tests
shell: python
run: python3 ${{ github.action_path }}
env:
INPUTS_CHANNEL: ${{ inputs.channel }}
INPUTS_CHANNEL_TEST_REPOSITORIES: ${{ inputs.channel_test_repositories }}
INPUTS_REPOSITORY: ${{ inputs.repository }}Note: I've renamed the test module to I haven't had a closer look into st-package-reviewer-action. It seems to use basic parts of package_control very much like packagecontrol.io does. I think we can easily replace it with latest PC4.0-beta modules even for use with current 3.0.0 schemes, but with all those bugs like basic auth fixed. Whether basic auth is needed, depends on how many API calls are needed during review process. Without authentication the limit is 60. 5000 otherwise. |
Not really necessary to use env variables. IMO that's better than env vars since it should be easier to use locally. |
|
Definitely. Just not that easy to handle as scheme reviewer monkey patches UnitTests in hard to follow ways, which requires removing arguments not meant for unittesting framework. That's somewhat crazy. Maybe it's even enough to run jsonschema using those from four-point-oh to catch most of the restrictions. |
We don't need to go through PC server, which may have issues and occasionally produces unexpected 500 errors that we are unable to debug. Instead, we should be able to perform the necessary checks through Github Actions entirely. That way we are independent of the PC server, don't consume API calls of the rate limit (and I can apply fixes myself instead of Will being the only one who can change things).
References:
The text was updated successfully, but these errors were encountered: