[ENT-12098] Add an initial implementation of an internal cfbs command to generate MPF release information #208
Conversation
… MPF release information Signed-off-by: jakub-nt <[email protected]>
|
Thank you for submitting a PR! Maybe @craigcomstock can review this? |
Signed-off-by: jakub-nt <[email protected]>
Signed-off-by: jakub-nt <[email protected]>
Signed-off-by: jakub-nt <[email protected]>
Signed-off-by: jakub-nt <[email protected]>
Signed-off-by: jakub-nt <[email protected]>
There was a problem hiding this comment.
Great work 🚀 I see you still have some # TODO's left to address. I have some smaller nitpicks for you. But in general:
- It's nice if you try to avoid too many nested
if's. They tend to make it really hard to understand the code. But this is not always the case. - When you look for the masterfiles tarball URL, maybe it's possible to search for it, instead of hard coding where it is in the JSON? Maybe there is a common denominator you can look for? I'm not sure.
I tried running your program, but it hung. Or maybe it just takes a very long time? I did not try to figure out why.
Looking forward to re-review!
| def versions_checksums_files( | ||
| files_dir_path, version, versions_dict, checksums_dict, files_dict | ||
| ): | ||
| for root, dirs, files in os.walk(files_dir_path): |
There was a problem hiding this comment.
Best practice is to use _ when we don't care about the value
| for root, dirs, files in os.walk(files_dir_path): | |
| for root, _, files in os.walk(files_dir_path): |
| """ | ||
|
|
||
| import os | ||
| import dictdiffer |
There was a problem hiding this comment.
We should consider avoiding this dependency. Otherwise we should add it to requirements.txt ?
| git_version_dict = git_versions_dict["versions"][version]["files"] | ||
|
|
||
| for diff in list(dictdiffer.diff(download_version_dict, git_version_dict)): | ||
| with open("differences/difference-" + version + ".txt", "w") as f: |
There was a problem hiding this comment.
Maybe you can open the files once (per version in versions) instead of opening them every time you want to append a line?
| does_match = True | ||
|
|
||
| for version in downloaded_versions: | ||
| print(version) |
There was a problem hiding this comment.
Is this a leftover debugging print?
|
|
||
| filename = url.split("/")[-1] | ||
| tarball_path = version_path / filename | ||
| urllib.request.urlretrieve(url, tarball_path) |
There was a problem hiding this comment.
Consider using fetch_url() from cfbs.utils. Otherwise, make sure to check status code
| def check_required_command(command): | ||
| if not shutil.which(command): | ||
| print("`%s` was not found" % command) | ||
| sys.exit(1) | ||
|
|
||
|
|
||
| def check_required_commands(commands): | ||
| for c in commands: | ||
| check_required_command(c) |
There was a problem hiding this comment.
These two can probably be the same function. If the passed argument is an interable, then it can recursively call itself for each element in the iterable.
| required_commands = ["git", "make", "automake", "autoconf"] | ||
| check_required_commands(required_commands) | ||
|
|
||
| # clone the MPF repo every time the script is run, in case there are updates |
There was a problem hiding this comment.
Can you not fetch and rebase instead?
| if os.path.isdir(MPF_PATH): | ||
| shutil.rmtree(MPF_PATH) | ||
|
|
||
| subprocess.run( |
There was a problem hiding this comment.
Maybe we should handle exceptions if the command fails? I'm not sure.
| versions_dict, checksums_dict, files_dict = initialize_vcf() | ||
|
|
||
| for tag in interesting_tags: | ||
| print(tag) |
No description provided.