Fix issue 826 - correct python and pip fallback #878

j0yu · 2020-04-18T16:35:19Z

Closes #826

Ran across #826 while trying to refactoring pip wiki fix PR #749. The CI for #749 is also added here since it requires the pip fallback fix

Use python2 or python3 rather than python

~~Currently with disgusting hack and thinking about either (need feedback!):~~
- ~~a new add_sys_path kwargs for ResolvedContext.get_environ and ResovledContext._execute, or~~
- a new ResolvedContext.append_sys_path attribute which ResovledContext._execute and any other methods can use
Fallback to more compatible pip version if possible

See AcademySoftwareFoundation#878 for CI check

nerdvegas · 2020-04-22T09:30:15Z

src/rez/pip.py

-            python_version,
-            pip_version=pip_version or "latest"
-        )
+    for version in [pip_version, pip_version or "latest"]:


won't this attempt to find the same pip version twice, when pip_version is specified?

nerdvegas · 2020-04-22T09:31:20Z

src/rez/pip.py


-    if not py_exe:
+    else:  # No break from: for version in [pip_version,...]


I can't believe I've been writing python this long and have never seen this. There's nothing technically wrong with this, but tbh I'd avoid it because it's so esoteric.

True, I also find it fairly susceptible to indent/whitespace messing up logic if code was reformatted/refactored incorrectly hence the comment. I'll see if I can change it up a bit

nerdvegas · 2020-04-22T09:35:19Z

src/rez/pip.py

+    # by RexExecutor.append_system_paths(), which calls Shell.get_syspaths()
+    shell_class = create_shell().__class__
+    current_syspaths = shell_class.syspaths
+    try:


Yeah I don't think we should do this. Not because it's a hack per se, but because it isn't threadsafe. Ie this could alter syspaths in a context being created in another thread.

nerdvegas · 2020-04-22T09:50:06Z

So wrt find_python_from_package:

Why recreate a context containing just python, rather than possibly [python, pip]? The pip package isn't going to provide a python exe so this seems unnecessary?
In terms of what to do about avoid system paths. We could fix this by providing some arg to execute_shell/etc like no_append_sys_paths. However it's not the right fix in the long run, when we (hopefully) have the more granular env-var control in place - once that's there, you should be able to do an execute_shell and redefine how any given var behaves anyway (eg inherits original value etc).

So 2. is basically your first suggestion, but I think no_append_sys_paths is a bit better naming since no_append_sys_paths=False is definitely default behaviour. I would go with this. Definitely not with the hack in place currently, see comments.

Cheers
A

See AcademySoftwareFoundation#878 for CI check

with disgusting hack

j0yu · 2020-04-22T14:37:58Z

Why recreate a context containing just python, rather than possibly [python, pip]? The pip package isn't going to provide a python exe so this seems unnecessary?

Good point, the logic can be much simpler now too!

but I think no_append_sys_paths is a bit better naming since no_append_sys_paths = False is definitely default behaviour. I would go with this.

Cool, I'll go with

a new ResolvedContext.non_append_sys_path attribute which ResovledContext._execute and any other methods can use

j0yu · 2020-04-22T14:42:26Z

Actually, I still prefer the name without the no_ prefix to avoid ~~DNEG~~ double negatives like in your example and also, to avoid statements like if not no_append_sys_path:

JeanChristopheMorinPerso · 2020-04-22T19:10:49Z

.github/workflows/installation.yml

+  linux-containers:
+    name: python:${{ matrix.python-version }} - ${{ matrix.method }}
+    runs-on: ubuntu-latest
+    container: python:${{ matrix.python-version }}


That runs the test in a docker container right? What's the reason for doing this? I can't think of a case where install rez would fail in a container but not on a bare metal/vm machine. But maybe I'm missing something.

I was just double checking that if the system already has Python it wasn't doing anything different from using actions/setup-python

It didn't, see here, so I'll remove it

JeanChristopheMorinPerso · 2020-04-22T22:13:14Z

.github/workflows/installation.yml

+    - name: Install
+      run: |
+        ${{ matrix.method }}
+        find /opt/rez/ -maxdepth 2


I'm tempted to suggest to use check-manifest to test that pip would install the correct files. Though, we would probably have to work a little bit on the MANIFEST.in to make it work. But that should be done in another PR IMO if we ever decide to go that way.

yeah, sounds like a good idea. Let's get a new PR/issue opened for that

JeanChristopheMorinPerso · 2020-04-22T22:24:37Z

.github/workflows/installation.yml

+          - '3.6'
+          - '3.7'
+        method:
+          - 'python ./install.py'


We technically already test this type of install quite a lot for all platforms and all python versions. I think it would make sense to test this here, but only if we test install.py more in depth, like make sure it installs the rez scripts in the rez folder correctly, and on all platforms. We should also test https://github.com/nerdvegas/rez/blob/master/install.py#L81.

Without testing these things, I have the feeling we will just have jobs that will take CPU time without much benefits.

What do you think?

test install.py more in depth, like make sure it installs the rez scripts in the rez folder correctly, and on all platforms. We should also test https://github.com/nerdvegas/rez/blob/master/install.py#L81.

I agree with testing python -E, let's do those in a separate PRs.

We technically already test this type of install quite a lot for all platforms and all python versions.

Does rez-selftest check python ./install.py? The other CI tests checks via python ./install.py ./build where as I wanted this to actually check the wiki's python ./install.py

Originally these tests were written just to double check what is said in the Installation Wiki page actually holds true and is not misleading beginners.

So far, this is just a start and we can expand on it to more in-depth and multi-platform tests in the future to ensure whenever someone discovers this project and tries to follow the wiki step by step, it should just work.

Awaiting AcademySoftwareFoundation#878

src/rez/pip.py

nerdvegas · 2020-04-28T05:24:21Z

Ok cheers, pretty much ready to merge this. Ran out of time just now but this will be done soon.
Thx
A

ps - let's keep separate issues (eg installation CI) in separate PRs ;) The smaller the PR the easier it is for me to digest and merge it.

j0yu · 2020-04-28T10:25:38Z

Cheers for the review! I've updated with changes to code comments.

ps - let's keep separate issues (eg installation CI) in separate PRs ;) The smaller the PR the easier it is for me to digest and merge it.

Sure thing, I'll keep that in mind next time!

src/rez/pip.py

Awaiting AcademySoftwareFoundation#878

See AcademySoftwareFoundation#878 for CI check

commit 8d51e29 Author: Joseph Yu <josephyu2712+github@gmail.com> Date: Mon Apr 27 15:16:37 2020 +0100 Emphasised pip CLI warnings commit e0d698e Author: Joseph Yu <josephyu2712+github@gmail.com> Date: Wed Apr 22 11:36:00 2020 +0100 Extra links to install from GitHub commit 13183cf Author: Joseph Yu <josephyu2712+github@gmail.com> Date: Wed Apr 22 10:56:48 2020 +0100 Updated CLI in rez-env warning commit e922172 Author: Joseph Yu <josephyu2712+github@gmail.com> Date: Sat Apr 18 17:39:40 2020 +0100 Just update Installation.md See AcademySoftwareFoundation#878 for CI check

j0yu added a commit to wwfxuk/rez that referenced this pull request Apr 18, 2020

Just update Installation.md

089aa8f

See AcademySoftwareFoundation#878 for CI check

j0yu marked this pull request as ready for review April 20, 2020 09:54

j0yu mentioned this pull request Apr 20, 2020

[wiki] Changes to pip install command #749

Open

nerdvegas reviewed Apr 22, 2020

View reviewed changes

j0yu added a commit to wwfxuk/rez that referenced this pull request Apr 22, 2020

Just update Installation.md

d3a8c7e

See AcademySoftwareFoundation#878 for CI check

j0yu added 7 commits April 22, 2020 15:08

Initial pip fallback logic from 5e180d9

2219ec2

Added installation CI

249255e

Initial find_python_from_package

622a14d

Working rez package python

7860662

with disgusting hack

pip_version, latest

ff7c608

using valid_found instead of for..else

304253b

Use existing pip-python context

e5a2ef3

j0yu added 2 commits April 22, 2020 15:49

Using new ResolvedContext.append_sys_path

6b30a49

Tidying, refernce to PRs

72f6a00

j0yu force-pushed the fix-826-pip-fallback branch from ce3c9da to 72f6a00 Compare April 22, 2020 15:07

JeanChristopheMorinPerso reviewed Apr 22, 2020

View reviewed changes

j0yu added 4 commits April 27, 2020 10:50

TESTING windows and macos

b36ca00

Removed linux containers, more win/mac tests

eddb4f2

TESTING more windows and mac

30a04db

Removed multi-platform tests for future PR

7faaa4b

j0yu added a commit to j0yu/bleeding-rez that referenced this pull request Apr 27, 2020

Initial multi-platform install CI

7e59c29

Awaiting AcademySoftwareFoundation#878

j0yu mentioned this pull request Apr 27, 2020

Initial multi-platform install CI #882

Closed

1 task

j0yu requested a review from JeanChristopheMorinPerso April 27, 2020 13:53

j0yu requested a review from nerdvegas April 27, 2020 13:53

nerdvegas reviewed Apr 28, 2020

View reviewed changes

src/rez/pip.py Outdated Show resolved Hide resolved

nerdvegas reviewed Apr 28, 2020

View reviewed changes

src/rez/pip.py Show resolved Hide resolved

nerdvegas approved these changes Apr 28, 2020

View reviewed changes

826 is issue not PR

6e781a5

nerdvegas reviewed Apr 30, 2020

View reviewed changes

src/rez/pip.py Show resolved Hide resolved

nerdvegas merged commit e599cb5 into AcademySoftwareFoundation:master Apr 30, 2020

j0yu deleted the fix-826-pip-fallback branch April 30, 2020 09:15

nerdvegas mentioned this pull request May 14, 2020

context sourcing broken (ResolvedContext.append_sys_path not serialised) #889

Closed

j0yu added a commit to j0yu/bleeding-rez that referenced this pull request Jul 14, 2020

Initial multi-platform install CI

a39eeb0

Awaiting AcademySoftwareFoundation#878

j0yu added a commit to wwfxuk/rez that referenced this pull request Jul 14, 2020

Just update Installation.md

53fcf9e

See AcademySoftwareFoundation#878 for CI check

j0yu added a commit to wwfxuk/rez that referenced this pull request Aug 5, 2020

Just update Installation.md

7417ccb

See AcademySoftwareFoundation#878 for CI check

j0yu added a commit to wwfxuk/rez that referenced this pull request Aug 5, 2020

Just update Installation.md

8a04242

See AcademySoftwareFoundation#878 for CI check

j0yu added a commit to wwfxuk/rez that referenced this pull request Aug 16, 2020

Just update Installation.md

dd1d7f8

See AcademySoftwareFoundation#878 for CI check

j0yu added a commit to wwfxuk/rez that referenced this pull request Sep 15, 2020

Just update Installation.md

5312239

See AcademySoftwareFoundation#878 for CI check

j0yu added a commit to wwfxuk/rez that referenced this pull request Mar 18, 2021

Just update Installation.md

e922172

See AcademySoftwareFoundation#878 for CI check

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 826 - correct python and pip fallback #878

Fix issue 826 - correct python and pip fallback #878

j0yu commented Apr 18, 2020 •

edited

Loading

nerdvegas Apr 22, 2020

nerdvegas Apr 22, 2020

j0yu Apr 22, 2020

nerdvegas Apr 22, 2020

nerdvegas commented Apr 22, 2020 •

edited

Loading

j0yu commented Apr 22, 2020

j0yu commented Apr 22, 2020

JeanChristopheMorinPerso Apr 22, 2020 •

edited

Loading

j0yu Apr 27, 2020

JeanChristopheMorinPerso Apr 22, 2020

j0yu Apr 27, 2020

JeanChristopheMorinPerso Apr 22, 2020

j0yu Apr 27, 2020 •

edited

Loading

nerdvegas commented Apr 28, 2020

j0yu commented Apr 28, 2020


		if not py_exe:
		else: # No break from: for version in [pip_version,...]

Fix issue 826 - correct python and pip fallback #878

Fix issue 826 - correct python and pip fallback #878

Conversation

j0yu commented Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerdvegas commented Apr 22, 2020 • edited Loading

j0yu commented Apr 22, 2020

j0yu commented Apr 22, 2020

JeanChristopheMorinPerso Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0yu Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

nerdvegas commented Apr 28, 2020

j0yu commented Apr 28, 2020

j0yu commented Apr 18, 2020 •

edited

Loading

nerdvegas commented Apr 22, 2020 •

edited

Loading

JeanChristopheMorinPerso Apr 22, 2020 •

edited

Loading

j0yu Apr 27, 2020 •

edited

Loading