Add python and git lfs check to bootstrap #26
Conversation
|
@aabtop you would be a good reviewer on this one! |
|
Thanks, this looks great! |
|
As you've no doubt noticed, there's a few rough edges here with regards to our CI running on PRs from forked repositories. Yours is the first btw, congratulations! I thought I addressed this and your latest merge would have fixed the issue, but apparently not. Unfortunately I can't dig in too much right now because I'm on a plane. I'll take another look later, but I would like to get this in soon. |
| if version.major != 3 or version.minor < 8 or version.minor > 10: | ||
| raise Exception("This script requires Python >=3.8,<=3.10") | ||
|
|
||
| def check_venv_status(venv_path): |
There was a problem hiding this comment.
I thought I addressed this and your latest merge would have fixed the issue, but apparently not. Unfortunately I can't dig in too much right now because I'm on a plane. I'll take another look later, but I would like to get this in soon.
Ahh, so after looking at the new errors more closely, they are real, and caused by the fact that in our build system we do create .venv directories without setting VIRTUAL_ENV.
I like the idea of the "venv is not active" check here, but there's a couple places where VIRTUAL_ENV shows up and I'm not sure it's worth tracking it all down.
Also, I'm realizing this late (sorry), but this is going to get run every time ./b is run, and while that's not a big deal for ./b data-snapshot run, some of the other commands are smaller and shorter, and this would be quite verbose for them.
I took your PR here and ran with it over in #33. I ended up removing the venv checks (though leaving the suggestion in the readme). Not trying to sidestep your PR here, just that your PR here highlighted some important issues that I wanted to get fixed sooner rather than later.
Also,
- I moved the
pip install -r requirements.txtcall into a different place such that it's only called when./b data-snapshot runis called. - I've also removed the lfs check since that's also slightly verbose to run every time
./bis called. Also, so long as you have git lfs installed, your files should be updated properly on every checkout so you shouldn't need to manually "git lfs fetch" or "git lfs checkout". If I'm missing something let me know, but I've left it out for now.
With #33 merged I think we can close this PR, but if you still think it's worth thinking more about I'm happy to discuss some of your other changes here that I had omitted.
There was a problem hiding this comment.
Ahh, nice! Didn't realize the build system was already creating .venv and makes sense we don't want to bloat the other./bcommands that are much shorter.
re git lfs: Oh interesting. When I did a git clone, it didn't automatically git lfs checkout, but agreed since it should only be a one time thing anyways, it doesn't need to be in the run script.
Changes look good to me! Appreciate the quick reply.
Closing the PR now
|
Solved in #33 |
requirements.txtrequire >=3.7,<3.11