AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
921 stars 329 forks source link

distlib parses version string instead of using version_info to make script names #1714

Closed cfxegbert closed 2 months ago

cfxegbert commented 3 months ago

Use version_info to determine python version when building scripts

JeanChristopheMorinPerso commented 3 months ago

Hi @cfxegbert, can you tell us more about the reason for this fix please? The changes you made are inside a vendored project that we do not own.

cfxegbert commented 3 months ago

When you install rez with install.py it creates the executable and the executable with the python major.minor version number

-rwxr-xr-x@ 1 root  wheel  232 Mar 13 10:42 rez
-rwxr-xr-x@ 1 root  wheel  232 Mar 13 10:42 rez-3.9
-rwxr-xr-x@ 1 root  wheel  252 Mar 13 10:42 rez-benchmark
-rwxr-xr-x@ 1 root  wheel  252 Mar 13 10:42 rez-benchmark-3.9
-rwxr-xr-x@ 1 root  wheel  242 Mar 13 10:42 rez-bind
-rwxr-xr-x@ 1 root  wheel  242 Mar 13 10:42 rez-bind-3.9
...

If you install with python-3.10 or later you will end up the files like rez-3.1 instead of rez-3.10.

The vendor library has already been modified by others so it is already divergent from the original source.

cfxegbert commented 3 months ago

It looks like a similar fix has be made upstream. https://github.com/pypa/distlib/blob/master/distlib/scripts.py#L298-L301

brycegbrazen commented 3 months ago

@cfxegbert, I see in the vendor folder's README.md file that the description is

"Updated (June 2019) to enable wheel distribution based installations."

I wonder if we can just update to the latest release of distlib and it'll kill 2 birds with one stone (so we don't have a divergent library in our vendor folder)?

cfxegbert commented 3 months ago

There is also https://github.com/AcademySoftwareFoundation/rez/issues/1668.

If we update distlib to the latest/greatest do we have the tests in place to verify it? Looks like distlib is used in install.py, src/rez/utils/pip.py, src/rez/pip.py, src/rez/bind/hello_world.py and src/rez/tests/test_pip_utils.py

cfxegbert commented 3 months ago

@cfxegbert, I see in the vendor folder's README.md file that the description is

"Updated (June 2019) to enable wheel distribution based installations."

Changes that are not upstream:

util.py
a195feaba (Allan Johns       2021-06-16 15:18:33 +1000  218)
de4737f98 (Stephen Mackenzie 2022-08-28 17:39:30 -0400  219)                         # https://github.com/AcademySoftwareFoundation/rez/pull/1092
8412dc1a2 (Blazej Floch      2021-06-03 17:10:56 -0400  220)                         # Some packages have a trailing comma which would break the matching
8412dc1a2 (Blazej Floch      2021-06-03 17:10:56 -0400  221)                         if not ver_remaining:
8412dc1a2 (Blazej Floch      2021-06-03 17:10:56 -0400  222)                             break
a195feaba (Allan Johns       2021-06-16 15:18:33 +1000  223)                         # /end pull/1092
a195feaba (Allan Johns       2021-06-16 15:18:33 +1000  224)
brycegbrazen commented 3 months ago

@cfxegbert Looks like it's fixed in this commit on the distlib GitHub. It's part of the v0.3.3 release.

JeanChristopheMorinPerso commented 2 months ago

Closing this since I don't think that it's the right way to fix this.