-
Notifications
You must be signed in to change notification settings - Fork 81
Add update tool #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add update tool #64
Conversation
|
Thank you! Looks like a very interesting and useful solution. I'm going to try it myself! |
|
Thanks @cecinestpasunepipe, please let me know what you think. I've added some more features in 2e09e83 and 6faacb7. Output should now show version changes and we now support editable git install updates too. |
|
If you don't mind me asking, what does this solve that a won't do? |
That's a valid question, thanks for asking @pyrco. This is primarily helpful for installations with either:
If you have installed dissect using a "regular" Unfortunately running We have had some hard to identify issues in the past due to some dependencies being out-of-date and some being up-to-date due to the described pip dependency resolving. This helper utility aims to remove these struggles once and for all by providing a unified known-good updating method, no matter the installation method (git+editable, pypi) or dependency definition (regular or modified) that was used. |
|
Hi @JSCU-CNI , please open an issue and attache this PR so we can schedule a review into our sprints. |
| from pathlib import Path | ||
| from typing import Iterator | ||
|
|
||
| from pip._vendor import tomli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this stable enough? Maybe add it as a dependency for Python versions below 3.11, and use tomllib when available?
| return res | ||
|
|
||
|
|
||
| def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe copy catch_sigpipe from dissect.target.
| current_version = module.get("version") | ||
|
|
||
| if previous_version != current_version: | ||
| print(f"{module_name} \x1b[31m{previous_version}\x1b[0m -> \x1b[32m\x1b[1m{current_version}\x1b[0m") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you format this with some constants for the color codes? And maybe respect NO_COLOR?
|
|
||
| def _run(cmd: str, verbose: int) -> subprocess.CompletedProcess: | ||
| """Wrapper for subprocess run command.""" | ||
| res = subprocess.run(cmd, shell=True, capture_output=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change calls to this to be shell=False? I don't think there's a need for it. The calls will look a bit "uglier" but unless it's absolutely necessary to use shell=True we should avoid it.
| black dissect | ||
| isort dissect | ||
|
|
||
| [testenv:lint] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update these to use ruff?
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
|
Hi @JSCU-CNI, can you address the review comments or otherwise say if this PR is still necessary for you? |
This PR adds an update utility to the dissect meta package and fixes #70. The goal is to provide users with one simple method of updating the dissect suite, no matter the installation method. The updater relies on the
pyproject.tomldependencies definition.We are open for better solutions. This script has worked well for us for some time, but perhaps we can find a better way to create a unified update solution.