Fix rez executables aren't removed in windows install #1259
Conversation
|
On windows there are also *-script.py files generated and i am not sure why to be honest. I am currently also removing them and they don't seem to be needed. Anyone knows right away? |
install.py
Outdated
| filepath = os.path.join(virtualenv_bin_path, name) | ||
| if os.path.isfile(filepath): | ||
| os.remove(filepath) | ||
| if not platform_.os.startswith("windows"): |
There was a problem hiding this comment.
might be worth simplifying by dropping the platform check, and just testing (looping over) for suffixes in ["", ".exe", "-script.py"]?
There was a problem hiding this comment.
True. Good point. Should not make a difference locally. Way better.
There was a problem hiding this comment.
Done. Would be good if you (or someone else could also test this on Linux as i currently can't). I don't see anything that might break on Linux, but still.
|
@instinct-vfx @nerdvegas We could probably add a unit test for this one. What do you think? |
|
We have https://github.com/nerdvegas/rez/blob/master/.github/workflows/installation.yaml. Right now it's linux only but it would be really easy to convert to a multi platform test. |
Yes that makes sense. The fact alone that this went undercover so long is a good argument for adding tests. Won't be able to get to this today or tomorrow am afraid (today has only 30 mins left and tomorrow is fully booked already). |
|
So going to get back to this asap. Just for me to clarify, the idea would be to take the above workflow/action, make it windows compatible and add a step to check for removal of the executables on both platforms? @JeanChristopheMorinPerso @nerdvegas |
|
That sounds like it @instinct-vfx . |
|
Thinking about this, it is a tad more involved as we need to update the CI workflow to be cross platform as well as add the installation unittest. I would prefer to split off the work for this from this fix if this is okay with everyone. Can still take on it. |
|
Okay, so i have started working on the tests, but that needs some more thought i think. The workflow now contains a minimal set of tests. Obviously it works for testing "does installing work, does pip installing work" and it adds a bit more. That said the tests required here are a bit more involved and i am unsure if it is a good idea to implement them in the workflow directly or as separate tests in the normal selftest tests (that are maybe only run when used in this workflow?) The one specifically needed here would be to check for all rez scripts existing in the /rez subfolder but not in /bin or /Scripts directly (but only for the install.py runs). We could do a minimal version checking for existance of one of the scripts and assuming the others are either there too or not, but that feels somewhat sketchy. Would love to hear some input. As a note: I am fighting the workflow in Github Actions for a total of 99 Commits now. It's really horrible to work with and the intricacies of Linux vs. Mac vs. Windows make this really cumbersome. I have now gotten to the last test of the workflow and that now fails miserably on Windows again, possibly due to rez-bind python being so broken on Windows and possibly some other issues. Also i think this is really starting to go beyond the scope of the initial PR. @JeanChristopheMorinPerso @nerdvegas Thoughts? |
|
I think we can aim for a minimal test suites that covers most things, which I think you already have right? I'd leave anything too complex (running rez-bind is something I would consider too complex) for later. |
|
See latest 4 commits. I removed the import check as proposed. The updated installation workflow is now running on both ubuntu, macOS and windows. It serves as a test for the following aspects:
I had to move all rez calls to include the |
…hub.com:instinct-vfx/rez into bug/1258_rez_executables_not_removed_on_windows
|
Umm, why is there no build triggered? |
|
I am completely confused now. The actions are triggered in my fork. I have no clue why. Also, after moving the changes from my test branch (to save this repo from the hundreds of commits this took) it fails. Even though the workflow is the same. |
|
Found the issue for the actions failing. But i still have no clue why they are not run here, but instead on my fork (https://github.com/instinct-vfx/rez/actions/runs/2198241049) |
|
Maybe try to remove the path filters? |
|
But they run on my fork? I mean i did create the other branch to specifically not cause all of the runs here and i swerved away from this branch. But why would it run this one in my fork? |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Alright, it looks like we sorted out why the actions were not running. |
|
Thanks again for the support @JeanChristopheMorinPerso! |
There was a problem hiding this comment.
This is really great work @instinct-vfx. We are just missing the check for the files in the bin directory, but I think it can be delayed to another PR.
Fix #1258