WIP: introduce the cache-files check#7
Conversation
enr0n
left a comment
enr0n
left a comment
There was a problem hiding this comment.
Thanks for the PR! This will be a great addition. This was a quick review so please ask if anything I suggested did not make sense.
Please also consider adding some tests for this.
|
|
||
|
|
||
| def _check_cache_files(context: Context, directory: str): | ||
| files = subprocess.check_output(["find"], cwd=directory).decode().splitlines() |
There was a problem hiding this comment.
It seems unnecessary to shell out to find here. Shouldn't os.walk or something do the trick?
There was a problem hiding this comment.
we should do this with transparent archive access instead: https://code.launchpad.net/~ubuntu-server/+git/ubuntu-helpers/+merge/500673
There was a problem hiding this comment.
So, for the dput hook, which is the case where we only have the .changes files, I went with some improved version of your transparent archive access, and for the CLI case, where we only have the source_dir, I went with Path.glob().
Thanks both for the input :-)
There was a problem hiding this comment.
FYSA, the CLI case can still have the changes file as context.
| # XXX this needs to be better defined | ||
| # It's currently needed to be able to extract the .dsc in the | ||
| # cache-files check, but that extraction should probably be handled | ||
| # more generally in that Context class. | ||
| self._changes_path = changes |
There was a problem hiding this comment.
I think it makes sense to make .dsc an attribute of Context, just like the changes file, source dir, etc. You can add some sensible defaults to try and derive it by default (see the logic for changes file).
There was a problem hiding this comment.
In the end, when implementing the "transparent access" that jj suggested, I didn't need the .dsc anymore, only the path to the .changes, so I implemented properly that property, and the dput entry-point is making use of it.
I guess this could spark further refactor where we could actually only provide the path, and the Context would then go read it to populate Context.changes, but I believe that would belong to some subsequent refactor. What do you think?
Hyask
force-pushed
the
skia/cache_files
branch
3 times, most recently
from
b7540b4 to
6ea1475
Compare
| for cache_file in [ | ||
| # General | ||
| "CACHEDIR.TAG", | ||
| ".direnv", | ||
| "/.git/", # XXX: that one is kind of a problem when `ubuntu-lint` is invoked as a CLI in a git repo. Maybe we should see to activate that only for the `dput` hook? | ||
| # Python | ||
| ".pyc", | ||
| ".egg-info", | ||
| ".tox", | ||
| "__pycache__", | ||
| ]: |
There was a problem hiding this comment.
to avoid reconstructing them and for faster lookups, this bad-name list could be defined outside the _get_bad_files function as a set. and there should be a easy way to extend this list, for example to add ruff_cache or mypy_cache files.
There was a problem hiding this comment.
By "easy way to extend", do you mean a configuration file of sorts? If so, that's probably out of scope for now, but I would eventually like to have something like that.
| def __init__( | ||
| self, | ||
| changes: str | deb822.Changes | None = None, | ||
| changes_path: str | Path | None = None, |
There was a problem hiding this comment.
Hm, this feels a bit redundant since changes already accepts a path (though maybe the type annotation should be updated to make that more clear).
I think I would rather do one of the following:
- Add a
filesparameter to context, and pass those paths directly in the dput hook withchanges.get_files(). Or; - Keep the
changes_pathproperty, but set it whenchangesis a path, and not raw data. Then, in the dput hook start passing the absolute file path withchanges.get_changes_file()instead of passing the raw data.
What do you think?
| try: | ||
| # Case where we directly have the source_dir | ||
| files = [str(p) for p in Path(context.source_dir).absolute().glob("**")] | ||
| print(files) | ||
| bad_files += _get_bad_files(files) | ||
| except MissingContextException: | ||
| pass | ||
|
|
||
| try: | ||
| # Case of a .changes file and we need to read the tarballs | ||
| changes_files = context.changes.get("Files") | ||
| if changes_files: | ||
| for item in changes_files: | ||
| if ".tar." in item["name"]: | ||
| file_path = context.changes_path.parent / item["name"] | ||
| with tarfile.open(file_path, "r") as tar: | ||
| bad_files += _get_bad_files(tar.getnames()) | ||
| except MissingContextException: | ||
| pass |
There was a problem hiding this comment.
Shouldn't these be mutually exclusive cases? Or, I guess there needs to be some consistency check as is done in other places where data can come from multiple context sources.
| pass | ||
|
|
||
| if bad_files: | ||
| context.lint_warn( |
There was a problem hiding this comment.
This should be lint_fail(). The callers can decide to downgrade or not. I.e. I see in the CLI you configured this to be warning by default.
| for cache_file in [ | ||
| # General | ||
| "CACHEDIR.TAG", | ||
| ".direnv", | ||
| "/.git/", # XXX: that one is kind of a problem when `ubuntu-lint` is invoked as a CLI in a git repo. Maybe we should see to activate that only for the `dput` hook? | ||
| # Python | ||
| ".pyc", | ||
| ".egg-info", | ||
| ".tox", | ||
| "__pycache__", | ||
| ]: |
There was a problem hiding this comment.
By "easy way to extend", do you mean a configuration file of sorts? If so, that's probably out of scope for now, but I would eventually like to have something like that.
3 participants
This is a port of that original MP.
Please have a look at all the
XXXcomments, they contain questions that need to be answered before even thinking about merging this.