setup: venv to manage python dependencies automatically#44
Conversation
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 3 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Pistonight).
setup_venv.py line 10 at r1 (raw file):
def enter_venv(): is_already_in_venv = not not os.environ.get("NX_DECOMP_TOOLS_IN_VENV") if is_already_in_venv:
Suggestion:
if os.environ.get("NX_DECOMP_TOOLS_IN_VENV"):setup_venv.py line 12 at r1 (raw file):
if is_already_in_venv: return expected_executable = Path(__file__).parent / ".venv" / "bin" / "python"
Suggestion:
venv_executable = Path(__file__).parent / ".venv" / "bin" / "python"setup_venv.py line 18 at r1 (raw file):
os.environ["NX_DECOMP_TOOLS_IN_VENV"] = "1" os.execv(expected_executable, [expected_executable, *sys.argv])
remove trailing spaces here (if some exist? Reviewable doesn't show me anymore)
setup_venv.py line 47 at r1 (raw file):
subprocess.check_call([ venv_pip, "install",
remove trailing spaces
viking/src/tools/check.rs line 644 at r1 (raw file):
let mut p = base_path; p.extend(["asm-differ", "diff.py"]); Ok(p)
Suggestion:
let differ_path_venv = base_path.join([".venv", "bin", "asm-differ"]);
// use the virtual env if one is setup with setup_python_venv()
if differ_path_venv.exists() {
return Ok(differ_path_venv)
}
// fallback to the .py entry point
Ok(base_path.join(["asm-differ", "diff.py"]))
Pistonight
left a comment
There was a problem hiding this comment.
@Pistonight made 5 comments.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on MonsterDruide1).
setup_venv.py line 18 at r1 (raw file):
Previously, MonsterDruide1 wrote…
remove trailing spaces here (if some exist? Reviewable doesn't show me anymore)
it did exist, removed
setup_venv.py line 47 at r1 (raw file):
Previously, MonsterDruide1 wrote…
remove trailing spaces
Done.
setup_venv.py line 10 at r1 (raw file):
def enter_venv(): is_already_in_venv = not not os.environ.get("NX_DECOMP_TOOLS_IN_VENV") if is_already_in_venv:
Done.
setup_venv.py line 12 at r1 (raw file):
if is_already_in_venv: return expected_executable = Path(__file__).parent / ".venv" / "bin" / "python"
Done.
viking/src/tools/check.rs line 644 at r1 (raw file):
let mut p = base_path; p.extend(["asm-differ", "diff.py"]); Ok(p)
You can't join with an iterator and calling .join repeatedly creates unnecessary allocations hence the manual extend call
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 1 file and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Pistonight).
viking/src/tools/check.rs line 644 at r1 (raw file):
Previously, Pistonight (Michael Zhao) wrote…
You can't join with an iterator and calling
.joinrepeatedly creates unnecessary allocations hence the manualextendcall
Ah, ChatGPT lied to me.
What about .join(".venv/bin/asm-differ")?
Pistonight
left a comment
There was a problem hiding this comment.
@Pistonight made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on MonsterDruide1).
viking/src/tools/check.rs line 644 at r1 (raw file):
Previously, MonsterDruide1 wrote…
Ah, ChatGPT lied to me.
What about.join(".venv/bin/asm-differ")?
Done - I usually don't join with / to avoid having mixed / and \ on Windows but i think we don't really care about Windows here
I started using
uvto manage python projects and it doesn't come with a nice way to install global pip packages. I think it would be nice to have the setup script make a python venv automatically and install the necessary dependencies, which will make the setup more streamlined as well. Another benefit is the wheel versions are lockedWith this change, downstream projects just need to insert this snippet in
tools/setup.pyat the top (before importing other modules - this will setup the venv andexecvinto the venv)Now people setting up the project just need to have python installed (to run the setup script itself), and a venv will be created at
tools/common/venvautomatically.A shim for
asm-differis created for viking to call. If this shim does not exist, it will fallback to the current behavior of trying to call diff.py using global python. This way the change is fully backward-compatible and everything should behave as-is if the above snippet is not used. Projects can opt-in to the new behavior by updating the setup script.This change is