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.

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

requirements command improvements, fixes issue 5755. #5757

Merged
merged 6 commits into from
Jun 30, 2023
Merged

Conversation

matteius
Copy link
Member

@matteius matteius commented Jun 28, 2023

Fixes #5755

The issue

The requirements command should have it all it needs from the lock file, but its re-using a requirementslib method that installs the requirements, which was a regression for the requirements command when we released requirementslib==3.0.0

The fix

Provide our own method and relocate the requirements command logic to the routines module.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@mgmarino
Copy link
Contributor

In the test Pipfile.lock in #5755, this generates the following:

-i https://pypi.org/simple
cffi==1.15.1
cryptography==41.0.1; python_version >= '3.7'
pycparser==2.21
"pyjwt[crypto] @ --editable git+https://git@github.com/jpadilla/pyjwt.git@7665aa625506a11bae50b56d3e04413a3dc6fdf8"

Definitely no network calls, but this fails to install with:

➜  pipenv_test pip --version
pip 23.0.1 from /xxx/mgmarino/.pyenv/versions/3.9.16/lib/python3.9/site-packages/pip (python 3.9)
➜  pipenv_test pip install -r requirements.txt
ERROR: Invalid requirement: "pyjwt[crypto] @ --editable git+https://git@github.com/jpadilla/pyjwt.git@7665aa625506a11bae50b56d3e04413a3dc6fdf8"
Could not split options: --editable git+https://git@github.com/jpadilla/pyjwt.git@7665aa625506a11bae50b56d3e04413a3dc6fdf8"

@matteius
Copy link
Member Author

@mgmarino Just pushed a fix for that -- thanks for catching it.

@matteius matteius closed this Jun 28, 2023
@matteius matteius reopened this Jun 28, 2023
@mgmarino
Copy link
Contributor

Ok, two more comments:

  • the enclosing "" are causing problems. i.e. instead of:
"pyjwt[crypto] @ git+https://git@github.com/jpadilla/pyjwt.git@7665aa625506a11bae50b56d3e04413a3dc6fdf8"

needs to be:

pyjwt[crypto] @ git+https://git@github.com/jpadilla/pyjwt.git@7665aa625506a11bae50b56d3e04413a3dc6fdf8

.

  • Also, this doesn't seem to respect the scheme, i.e. if ssh:// is listed it still gets exported as https://. This is important for us because we have ssh authentication to github. Example:

In Pipfile.lock:

        "our-module": {
            "editable": true,
            "extras": [
                "extra_one",
                "extra_two"
            ],
            "git": "ssh://git@github.com/company/our-module.git",
            "ref": "818b13269eac0753d0029ffe496f3a6c08614bf0"
        },

is exported as

our-module[extra_one,extra_two] @ git+https://git@github.com/company/our-module.git@818b13269eac0753d0029ffe496f3a6c08614bf0

instead of:

our-module[extra_one,extra_two] @ git+ssh://git@github.com/company/our-module.git@818b13269eac0753d0029ffe496f3a6c08614bf0

Co-authored-by: Michael Marino <mmarino@gmail.com>
@matteius matteius requested a review from mgmarino June 28, 2023 13:51
Copy link
Contributor

@mgmarino mgmarino left a comment

Choose a reason for hiding this comment

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

lgtm, works for the simple test case in #5755 and our "real" cases. My last suggestion should get the tests working again, I think there was only one failure. I will probably add some additional test cases, but we can do that in another PR. :-)

Co-authored-by: Michael Marino <mmarino@gmail.com>
@matteius
Copy link
Member Author

matteius commented Jun 28, 2023

Actually I should probably fix those tests and not have the space after the semi-colon, since that's technically more correct. We added the space a while back because of some weird side effect with the test framework hypothesis and upgrading pyparsing in requirementslib that was somehow solved by adding the space, but since this command is isolated, we should be more correct and fix the test assumptions.

@matteius matteius requested a review from oz123 June 28, 2023 21:55
@matteius matteius merged commit cb23498 into main Jun 30, 2023
@matteius matteius deleted the issue-5755 branch June 30, 2023 03:52
@mgmarino
Copy link
Contributor

Thanks, @matteius!

another-rex added a commit to google/osv.dev that referenced this pull request Jul 24, 2023
….12 (#1490)

Add --pull to the new build scripts, and pin pipenv version to 2023.6.12
for the deployment image.

Also remove ^ from package-lock that  was missed in the previous PR.

> We've pinned pipenv because pipenv requirements was behaving
differently on the most recent version, and we were matching on the
output to remove the requirement from the App Engine deployment:
> 
> in 2023.6.12 the output for the editable osv package was
> 
> -e ./../..
> in 2023.7.23 it was
> 
> ../../

This does not seem to be an intentional change from pipenv, as it's not
logged in the patch notes, but from bugs in their new implementation of
the requirements: pypa/pipenv#5757, so this
might change back in the future versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pipenv requirements now requiring installation of VCS modules
2 participants