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

Is it safe to pin setuptools/distribute/pip in requirements.txt? #6459

Open
jcushman opened this issue May 3, 2019 · 6 comments
Open

Is it safe to pin setuptools/distribute/pip in requirements.txt? #6459

jcushman opened this issue May 3, 2019 · 6 comments
Labels
project: vendored dependency Related to a vendored dependency state: needs discussion This needs some more discussion type: question User question

Comments

@jcushman
Copy link

jcushman commented May 3, 2019

  • Pip version: pip 19.1
  • Python version: Python 3.5.3
  • Operating system: macOS 10.14.4

As documented in this issue over at pip-tools, if you install packages like pytest or Markdown with --require-hashes you have to pin a specific version of setuptools as well, because it's included in their dependencies:

$ cat requirements.txt
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --generate-hashes
#
markdown==3.1 \
    --hash=sha256:fc4a6f69a656b8d858d7503bda633f4dd63c2d70cf80abdc6eafa64c4ae8c250 \
    --hash=sha256:fe463ff51e679377e3624984c829022e2cfb3be5518726b06f608a07a3aad680
$ pip install -r requirements.txt
Collecting markdown==3.1 (from -r requirements.txt (line 7))
  Using cached https://files.pythonhosted.org/packages/f5/e4/d8c18f2555add57ff21bf25af36d827145896a07607486cc79a2aea641af/Markdown-3.1-py2.py3-none-any.whl
Collecting setuptools>=36 (from markdown==3.1->-r requirements.txt (line 7))
ERROR: In --require-hashes mode, all requirements must have their versions pinned with ==. These do not:
    setuptools>=36 from https://files.pythonhosted.org/packages/ec/51/f45cea425fd5cb0b0380f5b0f048ebc1da5b417e48d304838c02d6288a1e/setuptools-41.0.1-py2.py3-none-any.whl#sha256=c7769ce668c7a333d84e17fe8b524b1c45e7ee9f7908ad0a73e1eda7e6a5aebf (from markdown==3.1->-r requirements.txt (line 7))

The pip-tools project does not pin setuptools without the scary-sounding --allow-unsafe flag, because "it may cause conflicts with pip itself". But the maintainers don't currently have a failing test case or know what the conflict would be. It also appears that setuptools was vendored by pip at some point, which might have changed the equation.

Can anyone on the pip side clarify under what circumstances it might be safe or unsafe to pin setuptools, and thus what the correct behavior would be for pip-compile --generate-hashes in the linked bug? To promote the use of --require-hashes, it would be great to either narrow pip-compile's definition of "unsafe", or provide clearer instructions about how to handle the unsafety when needed.

The other two packages on pip-compile's "unsafe" list are distribute and pip itself, so I guess the same question applies to those as well -- are there any risks as far as pip is concerned to pinning them if necessary to satisfy --require-hashes?

Thanks!

@uranusjr
Copy link
Member

uranusjr commented May 6, 2019

The pinning issue aside, why is a Markdown module depending on setuptools in the first place? :(

@jcushman
Copy link
Author

jcushman commented May 7, 2019

Python-Markdown requires setuptools>=36 because it wants to set markdown.__version__ = pkg_resources.extern.packaging.version.Version(...).

pytest requires setuptools>=40.0 because it wants to use py_modules in setup.cfg.

I don't know that either of those reasons was necessary, but are they a problem? It seems better to tell package authors "go ahead and depend on features of a minimum setuptools if you want to," rather than "be sure to support all versions of setuptools in use" -- otherwise new features of setuptools will take many years to be usable.

But if package authors are allowed to set minimum setuptools versions, then it also seems better (safer?) to pin that version in a requirements.txt file rather than letting each deployment end up with a different version.

@uranusjr
Copy link
Member

uranusjr commented May 7, 2019

I’d say there are better ways to do what the packages want to do. Maybe conceptually there is a case pip should handle itself (and setuptools) more gracefully in hash mode, or maybe pip-tools should allow pip/setuptools to be locked, that’s up for discussion. These particular usages do no make a strong case, however, IMHO.

@jcushman
Copy link
Author

jcushman commented May 7, 2019

The pytest usecase is pretty reasonable, isn't it? setuptools introduced an updated config file format and they want to use it -- that seems OK.

I poked around a bit more via a Google search on github. Some other popular packages setting minimum setuptools versions: kubernetes-client requires setuptools>=21.0.0; ipython requires setuptools>=18.5

I'm not sure it really matters how good the reasons are, does it? It's valid syntax; it's harmless, as far as anyone knows; it presumably solved a problem when introduced; and it's used in popular packages. Trying to convince package authors to never use it seems like a harder approach than tweaking pip-tools to handle it -- unless for some reason that wouldn't work.

@uranusjr
Copy link
Member

uranusjr commented May 8, 2019

I believe setup_requires or the PEP 518 build requirement is better for describing their intention 🙂

@uranusjr
Copy link
Member

uranusjr commented May 8, 2019

But regardless of whether Pytest can do better, I believe setuptools should be able to be pinned, since it does have a Python API (pkg_resources) that is useful for certain kind of projects. Whether pip-tools should allow pinning it, or pip should allow skipping it in hash mode, that needs to be discussed.

@pradyunsg pradyunsg added the S: needs triage Issues/PRs that need to be triaged label Jun 22, 2019
@chrahunt chrahunt added project: vendored dependency Related to a vendored dependency state: needs discussion This needs some more discussion type: question User question labels Jul 21, 2019
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Jul 21, 2019
inklesspen added a commit to inklesspen/mimir that referenced this issue Mar 12, 2021
diogoteles08 added a commit to diogoteles08/cryptography that referenced this issue Jul 4, 2023
The flag is needed to create hash-pinned requirements for pip and
setup-tools. Find more information about this at these issues from [pip-tools](jazzband/pip-tools#806) and from [pip](pypa/pip#6459).

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
reaperhulk pushed a commit to pyca/cryptography that referenced this issue Jul 11, 2023
)

* ci: Update GitHub owned actions to be referenced by SHA. Work automated using StepSecurity

Signed-off-by: StepSecurity Bot <[email protected]>

* ci: create hash-pinned requirements files for build and publish processes

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* ci: change ci files to install build and publish dependencies using hashes

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* ci: fix path to requirements files

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* ci: rebuild the requirement.txt files using `--allow-unsafe`

The flag is needed to create hash-pinned requirements for pip and
setup-tools. Find more information about this at these issues from [pip-tools](jazzband/pip-tools#806) and from [pip](pypa/pip#6459).

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* refactor(workflows): move build requirements files to a separated folder

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* fix(workflow): requirements download was erasing work from previous steps

Using the actions/checkout to download the requirements.txt was erasing
some necessary files that came from previous steps. Thus, this commit
changes moves the checkout action to the beginnig of the jobs.

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* ci: remove reference to inexistent input in pypi-publish.yml

* docs(workflows): remove comment related to a line already delated from code

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* refactor(workflows): use a workflow-level env var to define path to build requirements file

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* fix(workflows): refer to env vars using ${{  }} sintax

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* refactor(workflows): move build and publish requirements files

Moved from .github/workflows/requirements/ to .github/requirements/

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* docs(workflows): add comments on requirements files explaining their relation

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* ci(workflows): update build dependencies to match exactly the ones at pyproject.toml

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>

* ci: remove unnecessary parameter

When calling actions/checkout , we were passing the `ref` parameter as `github.ref`, but it will likely be always main, or the vary same value as the default for this parameter.

* Update dependabot config to cover build/publish dependencies

---------

Signed-off-by: StepSecurity Bot <[email protected]>
Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
Co-authored-by: StepSecurity Bot <[email protected]>
adRn-s added a commit to maxplanck-ie/parkour2 that referenced this issue Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: vendored dependency Related to a vendored dependency state: needs discussion This needs some more discussion type: question User question
Projects
None yet
Development

No branches or pull requests

4 participants