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

Support VCS requirements 1st class #1562

Closed
jsirois opened this issue Jan 9, 2022 · 5 comments · Fixed by #1563
Closed

Support VCS requirements 1st class #1562

jsirois opened this issue Jan 9, 2022 · 5 comments · Fixed by #1563
Assignees

Comments

@jsirois
Copy link
Member

jsirois commented Jan 9, 2022

Right now Pex requirement parsing lumps these under URLRequirement precluding any special handling required. One way or another #1556 will require recognizing VCS requirements.

@jsirois
Copy link
Member Author

jsirois commented Jan 9, 2022

These VCS requirements can come in two forms:

  1. The more limited, but now tandard PEP-440 direct reference form: https://www.python.org/dev/peps/pep-0440/#direct-references
  2. The original more full-featured Pip proprietary form: https://pip.pypa.io/en/stable/topics/vcs-support/

@jsirois
Copy link
Member Author

jsirois commented Jan 10, 2022

In case we need support for cloning using these requirements, some notes:

  1. svn:

    # Checkout the trunk from r1729.
    $ svn checkout http://svn.example.com/svn/repo/trunk@1729 trunk-1729
  2. hg:

    # Checkout revision 0.
    $ hg clone https://www.mercurial-scm.org/repo/hello#0 hello-0
  3. bzr:

    # Checkout the 2.6 branch from revision 6577
    $ bzr branch lp:bzr/2.6 -r 6577 bzr-2.6@6577

@cognifloyd
Copy link

What if you only supported locking PEP 440 style VCS requirements?
I'm happy to only use PEP 440 style VCS requirements. That could be a good way to push people away from pip's proprietary format if they want pex to lock stuff.

@jsirois
Copy link
Member Author

jsirois commented Jan 10, 2022

@cognifloyd the work to support Pip proprietary VCS requirements was done long ago. You might notice in #1563 there are no instances of 2 ~parallel changes. The PEP-440 direct reference and Pip proprietary url handling are already unified. There is a test edited there that shows the fact both are handled.

@jsirois
Copy link
Member Author

jsirois commented Jan 10, 2022

Ah - you were thinking this has to do with lockability. It does not. Both forms are equally unlockable without special effort since both forms can carry floating contents. Although PEP-440 decrees SHOULDs (https://datatracker.ietf.org/doc/html/rfc2119#section-3), they are just that, reccomendations that may be bypassed in practice. As such, Pex must parse a commit id from either format (which both formats support) and:

  1. If the commit id is not present: fail.
  2. If the commit id is present but not cryptographic: fail.

Instead of failing though, it could just pre-clone the repo and hash its contents. This would allow branches and tags to be useable in locks safely and it would also allow non-vcs local project requirements to be useable in locks safely (today these are rejected outright by pex3 lock). Beyond the floating issue lies the issue that Pip does not handle VCS requirements in hash checking mode; so Pex will need to do the hash checking itself. This can only be done afaict by Pex pre-cloning, building wheels and using --find-links substituting pinned reqs for the original VCS reqs.

jsirois added a commit that referenced this issue Jan 11, 2022
The current pex lock infrastructure cannot handle VCS requirements in
the same way it can't handle local project requirements. Unlike local
project requirements though, the failure to handle these requirements
was uncontrolled. Add explicit parsing of VCS requirements to allow them
to be singled out and rejected along side local project requirements.

This addresses the UX for the failure noted in #1556 but may not be the
final word there. If there is a desire to actually handle VCS
requirements in locks, the `VCSRequirement` parsing added here can be
expanded to extract the needed extra data (commit id certainly) to build
that support.

Closes #1562
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 a pull request may close this issue.

2 participants