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

3.44.24 breaks AppVeyor Python 3.4 builds #1091

Closed
engnadeau opened this issue Jan 28, 2018 · 11 comments
Closed

3.44.24 breaks AppVeyor Python 3.4 builds #1091

engnadeau opened this issue Jan 28, 2018 · 11 comments
Assignees
Labels
tests/build/CI about testing or deployment *of* Hypothesis

Comments

@engnadeau
Copy link

engnadeau commented Jan 28, 2018

C:\Users\appveyor\AppData\Local\Temp\1\pip-build-fv550ko8\hypothesis\setup.py:39: UserWarning: This version of setuptools is too old to correctly store conditional dependencies in binary wheels.  For more info, see:  https://hynek.me/articles/conditional-python-dependencies/
      'This version of setuptools is too old to correctly store '
    C:\Python34\lib\distutils\dist.py:260: UserWarning: Unknown distribution option: 'python_requires'
      warnings.warn(msg)
    error in hypothesis setup command: 'install_requires' must be a string or list of strings containing valid project/version requirement specifiers; Expected version spec in enum34; python_version=="2.7" at ; python_version=="2.7"
    
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in C:\Users\appveyor\AppData\Local\Temp\1\pip-build-fv550ko8\hypothesis\
Command exited with code 1
@alexwlchan
Copy link
Contributor

Thanks for the report – this looks like an issue introduced in #1090.

@Zac-HD I won’t pretend to understand packaging intricacies detail, but it looks like this warning only applies when we're building a binary wheel, but fires whenever you install Hypothesis? In which case we should probably move the warning behind an additional check, say:

if setuptools_version < (36, 2) and 'bdist_wheel' in sys.argv:
    ...

@Zac-HD
Copy link
Member

Zac-HD commented Jan 28, 2018

Ah, it looks like you're updating pip but not setuptools - see here. In short, that works for binary but not source distributions - replace

	- "%PYTHON%\\python.exe -m pip install wheel"

in appveyor.yml with

	- "%PYTHON%\\python.exe -m pip install --upgrade pip setuptools wheel"

The error message can and should explain how to fix the problem though, and that's on me - sorry!

@Zac-HD
Copy link
Member

Zac-HD commented Jan 29, 2018

TLDR: packaging sucks but you need the details to make good decisions 😭. Keep reading.

  1. pip>=1.4 can install distribution packages with a conditional dependency in install_requires, as described in PEP 508 and Hynek's blog post.

    Side note: some older packaging tools in the wild are "PEP 426 compliant" (eg wheel docs) and a few only understand == or != comparisons in environment markers. This has never been an accepted PEP, but I therefore encourage Python-2-specific dependencies to use =='2.7' rather than <'3'. (and yes, this is terrible)

  2. However, very old versions of setuptools (I don't know exactly which) choke on this metadata and thus fail to install source distributions. @alexwlchan - that, rather than the warning, is the true source of this issue. @hynek, it would be great to add a note about this to your excellent blog post to warn others 😕.

  3. When confronted with this problem, cryptography (Replace branches in setup.py with env markers pyca/cryptography#3508 and Conditional dependencies use a deprecated(?) format pyca/cryptography#4090) decided to stick with the older marker style in extras_require. This is more widely compatible with old packaging tools in the wild, but not supported reliably by Pipenv (Conditional dependencies/markers are ignored when locking pypa/pipenv#1303, (feature request) More support for conditional installation pypa/pipenv#1353) which is being pushed as the future of Python packaging.

We therefore have to choose between supporting the old and supporting the new.

I prefer new, in part because python -m pip install --upgrade pip setuptools wheel is a trivial step before retrying installation of Hypothesis. However, I'd prefer not to break end-users without good reason! I therefore suggest a compromise:

  • We put pip, setuptools, and wheel in our tools requirements to ensure we (and therefore PyPI) have new versions that build in the correct metadata.
  • We check that setuptools version is >= 36.2.1 before adding a conditional dependency in install_requires. If it is, carry on; if not:
    • If $TRAVIS_REPO_SLUG starts with HypothesisWorks, fail the job. We definitely don't want to release a version with bad tags.
    • If 'bdist_wheel' in sys.argv, also error out immediately. We don't want to let other people build bad wheels either, though I could be talked down from this.
    • If setuptools is old, but >= 18.0, conditional dependencies are supported in extras_require. Print a stark warning, with links to Hynek's post and the package installation tutorial on upgrading, then fall back to the older style in extras_require.
    • If their setuptools version is older than 18.0 (released June 2015), print the warning with an additional note about why conditional dependencies matter - and then error out immediately. We could instead use unconditional dependencies that are correct for the building version of Python, but any cached sdists would then be wrong for other versions. I'm sympathetic to old versions, but not enough to do the wrong thing.

Thoughts?

@hynek
Copy link
Contributor

hynek commented Jan 29, 2018

As for source distributions: they one upside: you get setup_requires. Could this be solved by setting it to the lowest setuptools that supports PEP 508? Or am I missing something?

🔥⭕️🔥 (← this shall be a tire fire)


Sadly it’s true that the brief tranquility of Python packaging not being entirely terrible came to a screeching end with having Pipenv only supporting only a subset while a large part of libs only support another one.

It tried to push for changes but I gave up because it’s obvious that it’s leading nowhere; everyone is right in a way and they won’t move.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 30, 2018

TLDR I'm sticking with my proposal above.

As for source distributions: they one upside: you get setup_requires. Could this be solved by setting it to the lowest setuptools that supports PEP 508? Or am I missing something?

Unfortunately, you are - setup_requires cannot require anything to do with setuptools itself, because this is only discovered after setuptools is executing.

I did some more reading, and bounced off setuptools docs to PEP 518 ("Specifying Minimum Build System Requirements for Python Projects"). Conveniently, this adds another build file (pyproject.toml)... but of course if our toolchain is old enough to need this, we can't use it!

⛽️🔥⭕️🔥

Jepsen "tire fire" image

(h/t Jepsen)

Abandon hope, etc.


To see any real progress we'll need maintainers and/or users to abandon old toolchains, which is obviously not going to be popular with the holdouts. pipenv also needs to either support libraries better or acknowledge that it doesn't, so that concerns like "works on another machine/version/OS/etc." have somewhere to live 😞

@Zac-HD Zac-HD self-assigned this Jan 30, 2018
@Zac-HD Zac-HD added bug something is clearly wrong here tests/build/CI about testing or deployment *of* Hypothesis labels Jan 30, 2018
@Zac-HD
Copy link
Member

Zac-HD commented Feb 12, 2018

Note to self: per #1106, I should add tests_require to setup.py while I'm in the area.

@dchudz
Copy link
Member

dchudz commented Feb 12, 2018

Oops, I don't think I really meant tests_require. What would actually help is a extras_require= with a dev or test section like this. Then we can do pip install -e .[dev]. E.g. replacing the pip install . in guides/internals.rst (after #1100) with this would mean you can skip the following two pip installs.

@dchudz
Copy link
Member

dchudz commented Mar 5, 2018

If we do add an extras_require, I'd try to do it in a way that parses requirements/tools.txt, to avoid duplication. (Also I think it's totally fine not to. Our contributing guide is pretty clear now.)

@Zac-HD
Copy link
Member

Zac-HD commented Mar 5, 2018

See 'Developing Reusable Things' - we can instead generate the pinned requirements from setup.py, via the .in files depending on -e ..[dev], for example.

@hynek
Copy link
Contributor

hynek commented Mar 8, 2018

FTR it seems like Pipenv fixed extras_require conditionals.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 27, 2018

Closing this as wontfix - having done some more work, I just don't think correctness is worth the cost in compatibility with old versions at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests/build/CI about testing or deployment *of* Hypothesis
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants