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

Various requirements fixups #4895

Merged
merged 17 commits into from
Apr 1, 2020
Merged

Various requirements fixups #4895

merged 17 commits into from
Apr 1, 2020

Conversation

blag
Copy link
Contributor

@blag blag commented Mar 24, 2020

Background

I wanted to specify python_version < '3.4' for the singledispatch dependency in #4894, but the fixate-requirements.py script does not support what pip calls "markers". Markers are environment specifications that instruct pip when to install, or more importantly, when not to install a dependency.

Here's what a Python version marker would look like for #4894:

singledispatch==3.4.0.3 ; python_version < '3.4'

That marker would only install the singledispatch dependency if installing for Python < v3.4. The singledispatch package is a backport of the same feature for Python 3.4+, available in the standard library in the functools package. So installing it for Python 3.4+ is not necessary.

Description

This PR:

  • Upgrades fixate-requirements.py to write out dependency markers if present in fixed-requirements.txt or in-requirements.txt.
  • Tweaks some Makefile output to be more readble
  • Adjusts some Makefile dependencies so multiple make invocations aren't constantly ping-ponging between different versions of pip, setuptools, etc. (read: speeds up the build)
  • Alphabetizes fixed-requirements.txt (in two groups) so it's easier to visually search the file for a dependency by name
  • (unrelated) Fixed backtick syntax for ReStructured Text (eg: not Markdown)

Future Work

I ran make requirements after these changes to ensure that the behavior of the script was no different than before. And I will be adding the environment marker to #4894 if this is merged in before that.

@blag blag added this to the 3.2.0 milestone Mar 24, 2020
@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Mar 24, 2020
Copy link
Member

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

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

Do we care about or support python < 3.4?

Are there any other uses for these "markers" that we should support in the future?

Makefile Show resolved Hide resolved
@blag
Copy link
Contributor Author

blag commented Mar 24, 2020

Note that the Travis tests are actually passing. The GitHub test status is not getting updated with the new results.

@blag blag force-pushed the various-requirements-fixups branch from 544dc92 to ec1f295 Compare March 25, 2020 17:46
@blag
Copy link
Contributor Author

blag commented Mar 26, 2020

There is a list of markers in PEP 508:

Marker Python equivalent Sample values
os_name os.name posix, java
sys_platform sys.platform linux, linux2, darwin, java1.8.0_51 (note that "linux" is from Python3 and "linux2" from Python2)
platform_machine platform.machine() x86_64
platform_python_implementation platform.python_implementation() CPython, Jython
platform_release platform.release()` 3.14.1-x86_64-linode39, 14.5.0, 1.8.0_51
platform_system platform.system()` Linux, Windows, Java
platform_version platform.version() #1 SMP Fri Apr 25 13:07:35 EDT 2014,
Java HotSpot(TM) 64-Bit Server VM,
25.51-b03, Oracle Corporation,
Darwin Kernel Version 14.5.0: Wed Jul 29 02:18:53 PDT 2015; root:xnu-2782.40.9~2/RELEASE_X86_64
python_version '.'.join(platform.python_version_tuple()[:2] 3.4, 2.7
python_full_version platform.python_version() 3.4.0, 3.5.0b1
implementation_name sys.implementation.name cpython
implementation_version see definition below 3.4.0, 3.5.0b1
extra An error except when defined by the context interpreting the specification. test

The implementation_version marker variable is derived from sys.implementation.version:

def format_full_version(info):
    version = '{0.major}.{0.minor}.{0.micro}'.format(info)
    kind = info.releaselevel
    if kind != 'final':
        version += kind[0] + str(info.serial)
    return version

if hasattr(sys, 'implementation'):
    implementation_version = format_full_version(sys.implementation.version)
else:
    implementation_version = "0"

@blag blag force-pushed the various-requirements-fixups branch from b70f473 to a2d7a37 Compare March 26, 2020 07:37
@blag
Copy link
Contributor Author

blag commented Mar 26, 2020

With #4894 not being merged this isn't strictly necessary. And with ST2 v3.3 dropping support for Python 2.7, we won't have to worry as much about Python versions when specifying our dependencies.

However, this PR does support a fairly new part of the requirements.txt standard, and it could still be useful in the future.

For instance, while StackStorm doesn't run on macOS, we could at least build the st2 client on macOS, and we could at least help make requirements run successfully on macOS by marking the pyinotify dependency only for Linux.

Currently when you try to run make requirements on macOS you get:

Collecting pyinotify
  Using cached https://files.pythonhosted.org/packages/e3/c0/fd5b18dde17c1249658521f69598f3252f11d9d7a980c5be8619970646e1/pyinotify-0.9.6.tar.gz
    ERROR: Command errored out with exit status 1:
     command: .../st2/venv/bin/python3 -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/ls/_2q7p19s2n1c3xdnphq4btzr0000gn/T/pip-install-ctf42xh_/pyinotify/setup.py'"'"'; __file__='"'"'/private/var/folders/ls/_2q7p19s2n1c3xdnphq4btzr0000gn/T/pip-install-ctf42xh_/pyinotify/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base pip-egg-info
         cwd: /private/var/folders/ls/_2q7p19s2n1c3xdnphq4btzr0000gn/T/pip-install-ctf42xh_/pyinotify/
    Complete output (1 lines):
    inotify is not available on macosx-10.9-x86_64
    ----------------------------------------
ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.

But if we marked pyinotify as being only for Linux:

pyinotify==0.9.6 ; platform_system == 'Linux'

Then we can at least run make requirements to completion:

Ignoring pyinotify: markers 'sys_platform == "Linux"' don't match your environment

@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Mar 26, 2020
@blag blag force-pushed the various-requirements-fixups branch from a2d7a37 to 3aebb17 Compare March 26, 2020 07:44
@blag
Copy link
Contributor Author

blag commented Mar 26, 2020

Waiting on https://gitlab.com/python-devs/importlib_metadata/-/merge_requests/118 to stop breaking the build.

Nope, not their bug.

Makefile Show resolved Hide resolved
Copy link
Member

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

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

LGTM!

@blag blag requested a review from arm4b March 31, 2020 04:42
@blag
Copy link
Contributor Author

blag commented Mar 31, 2020

Expands on the work done in #4750.

@punkrokk punkrokk self-requested a review March 31, 2020 20:11
Copy link
Member

@punkrokk punkrokk left a comment

Choose a reason for hiding this comment

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

LGTM.

@blag This is good stuff. I appreciate the cluster that this type of dependencies can create.

@blag blag merged commit 20ab6b6 into master Apr 1, 2020
@blag blag deleted the various-requirements-fixups branch April 1, 2020 04:27
@blag
Copy link
Contributor Author

blag commented Apr 1, 2020

Pulled in a few commits from #4896.
Closes #4896.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance performance size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants