Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[wiki] Changes to pip install command #749

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[wiki] Changes to pip install command #749

wants to merge 1 commit into from

Conversation

j0yu
Copy link
Contributor

@j0yu j0yu commented Sep 20, 2019 •
edited
Loading

Installation

Update the Wiki Installation page based off some PR feedback/comments:

Notable changes

  • Updated Installation Via Pip wiki page
  • New Installations GitHub CI Workflow Installation
  • src/rez/pip.py
    • Added PIP_SPECIFIER and _check_found() for pip version checking
    • find_pip() can fully fall back to rez install's virtualenv pip if any version/Python criteria are not satisfied
  • StringIO fixes that was breaking the install process

@j0yu j0yu force-pushed the docs/pip-install branch 4 times, most recently from 0766cde to 3216bec Compare September 20, 2019 15:57
@j0yu j0yu force-pushed the docs/pip-install branch 4 times, most recently from a238328 to 93f4477 Compare November 8, 2019 00:39
@nerdvegas
Copy link
Contributor

Hey thanks, just getting to this now.

I like the idea of an extra workflow for testing all installation types (via install.py, pip with various args). But a couple of things:

  • The name. What does wiki-doctest-installation mean? I would be happy to simply call this workflow 'Installation'
  • The workflow doesn't need to be docker-based, I would drop that.
  • I would also drop any use of or references to rez-bind, I want to deprecate this. I don't think we need to do the rez-bind pip in the workflow - we can just let rez-pip pickup the system pip install.
  • I also think these tests should verify the installation, rather than just run find. Eg, after a pip install ., I would run python and test that rez imports successfully. After rez-pip install ., I would verify that rez-env rez -- python -c 'import rez' works.

Looking forward to getting this merged, it's been mentioned in the past that installation testing is missing.

j0yu pushed a commit to wwfxuk/rez that referenced this pull request Dec 18, 2019
Initial addressing of PR comments
914bb1d 
AcademySoftwareFoundation#749 (comment)

- Renamed to Installation
- Non-docker python based
- Removed rez-bind, new rez env rez test
@j0yu j0yu force-pushed the docs/pip-install branch 18 times, most recently from 293c431 to 06ce840 Compare December 19, 2019 16:59
@j0yu
Copy link
Contributor Author

j0yu commented Feb 17, 2020

Looks like I missed out a few things during refactor/rebase.

Will need to update again from https://github.com/wwfxuk/rez/tree/pip-fallback

@j0yu j0yu mentioned this pull request Apr 18, 2020
Merged
2 tasks
@j0yu j0yu force-pushed the docs/pip-install branch from fe62e5a to 089aa8f Compare April 18, 2020 16:40
@j0yu
Copy link
Contributor Author

j0yu commented Apr 20, 2020

Should be good to review again @nerdvegas , I've moved the CI into #878 as it has fix for pip in there

JeanChristopheMorinPerso
JeanChristopheMorinPerso reviewed Apr 22, 2020
View reviewed changes
wiki/pages/Installation.md Outdated

However, these methods comes with a caveat - rez command line tools _are not
guaranteed to work correctly_ once inside a rez environment (ie after using the
`rez-env` command). The reasons are given in the next section.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really minor: I would add that a warning is outputted to the terminal when the rez commands are used in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mentioned a few lines below it but I'll tidy it up into 1 warning message

@nerdvegas
Copy link
Contributor

nerdvegas commented Apr 22, 2020 •
edited
Loading

Lets get onto this once #878 is sorted.

On a related note - yeah we should add a pypi build and have that being updated automatically. But I'm also still thinking about removing the cli tools from the pip install completely. There just seems to be scope for confusion/bug reports when the tools can be run from two different types of installation.

@j0yu j0yu force-pushed the docs/pip-install branch from 68283c7 to 7dffdc2 Compare April 22, 2020 14:07
@JeanChristopheMorinPerso
Copy link
Member

Umm ok, it seems like you pushed some commits. I'll have to find how to remove my approval...

JeanChristopheMorinPerso
JeanChristopheMorinPerso reviewed Apr 22, 2020
View reviewed changes
wiki/pages/Installation.md Outdated

- Add `/opt/rez/bin` to `PATH`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should invite users to add this to their path as it could make them think they can use the rez cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll strongly emphasis this and put the warning right after it

@JeanChristopheMorinPerso JeanChristopheMorinPerso dismissed their stale review April 22, 2020 22:38

New commits were made since my approval

@j0yu j0yu mentioned this pull request Apr 27, 2020
Closed
1 task
@j0yu j0yu force-pushed the docs/pip-install branch from 358cb35 to a44a215 Compare July 14, 2020 12:37
@j0yu j0yu force-pushed the docs/pip-install branch 2 times, most recently from 231c9e9 to 29c5885 Compare August 5, 2020 16:24
@j0yu j0yu force-pushed the docs/pip-install branch from 29c5885 to d7a16dd Compare August 16, 2020 09:00
@j0yu j0yu force-pushed the docs/pip-install branch from 7ecf5b7 to 8d51e29 Compare March 18, 2021 15:56
@j0yu j0yu force-pushed the docs/pip-install branch 2 times, most recently from 23fc3e3 to 3f57791 Compare March 30, 2021 17:13
@j0yu j0yu force-pushed the docs/pip-install branch from 3f57791 to 4a9947a Compare June 4, 2021 09:44
@j0yu j0yu force-pushed the docs/pip-install branch from 4a9947a to b580791 Compare July 9, 2021 13:06
@j0yu j0yu force-pushed the docs/pip-install branch from b580791 to 056f3d4 Compare July 21, 2021 11:56
@j0yu j0yu force-pushed the docs/pip-install branch from 056f3d4 to 91b16fe Compare August 3, 2021 08:59
Squashed commit of: 2.77.1..8d51e29
1bc40b3 
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 j0yu force-pushed the docs/pip-install branch from 91b16fe to 1bc40b3 Compare August 19, 2021 09:19
@maxnbk maxnbk requested review from nerdvegas and a team as code owners November 15, 2022 23:58
@JeanChristopheMorinPerso JeanChristopheMorinPerso requested a review from a team as a code owner April 4, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeanChristopheMorinPerso JeanChristopheMorinPerso Awaiting requested review from JeanChristopheMorinPerso

@nerdvegas nerdvegas Awaiting requested review from nerdvegas

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@j0yu @nerdvegas @JeanChristopheMorinPerso