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

setup.py: do not tighten dateutil for py27 and up #1406

Closed
wants to merge 2 commits into from

Conversation

blueyed
Copy link

@blueyed blueyed commented Mar 14, 2018

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #1406 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1406      +/-   ##
===========================================
- Coverage    80.61%   80.59%   -0.03%     
===========================================
  Files           87       87              
  Lines        12123    12123              
===========================================
- Hits          9773     9770       -3     
- Misses        2350     2353       +3
Impacted Files Coverage Δ
botocore/credentials.py 98.4% <0%> (-0.4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1048fc4...3e30efd. Read the comment docs.

@joguSD
Copy link
Contributor

joguSD commented Mar 14, 2018

Travis will pass for this because the requirements.txt wasn't also updated which is what we use when we run travis. So v2.6.1 was still used to run the test on Python 3.6.

This is something that we're currently looking into doing. I have a branch that will do this (and also updates setup.cfg and requirements.txt as well). The issue is that we're failing one test on Python 3.6 in travis. I've been testing different permutations to see what exactly is causing the recursion limit to be hit and it seems to be a combination of:

  • manipulating stdout (like travis or other CI systems tend to do)
  • dateutil logging a warning that gets intercepted by nosetests xunit plugin specifically the Tee class
  • Running BOTH the unit and functional tests together (running just the functional tests didn't seem to trigger this failure).

Add it to scripts/ci/install / requirements{,26}.txt, too.
@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

@joguSD
I've added a fixup to address this (would be nice to have a single point to configure this, i.e. setup.py), and it works locally.

@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

Do you have a link to a failing build?

@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

And to be honest: while I am really on the side of supporting legacy (Python) versions, what is the reason for not dropping py26 here also?

@joguSD
Copy link
Contributor

joguSD commented Mar 14, 2018

Heres a failing build: https://travis-ci.org/joguSD/botocore/builds/353493633
I've made a few changes as previously the real exception would be swallowed.

In the longer term I think we would like to drop Py26 support but we won't be able to do so for a long while as we still have customers expecting botocore (even more so for the aws-cli) to work on 2.6. The expectation is that these tools will work on Python 2.6 and without a proper warning and deprecation campaign there's not much we can do at the moment.

@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

I see, thanks for the explanations so far.
btw: can you cancel https://travis-ci.org/boto/botocore/builds/353553219 - it seems like Travis got stuck on the [ci skip] (it should not have created a build for it in the first place).

Feel free to close this PR (but hopefully for some strange reason it will work now - it works locally at least, but you say that it is related to Travis only, right?), or include it in your branch.

@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

Regarding the RecursionError in nose: are there plans to use pytest instead of node maybe?

@joguSD
Copy link
Contributor

joguSD commented Mar 14, 2018

I cancelled that build. Not sure why it got stuck.
We've discussed migrating to pytest and do use it on some of our other projects but there would be a non-trivial amount of effort to migrate our current test suite to be pytest compatible so it hasn't really been a priority.

@joguSD
Copy link
Contributor

joguSD commented Mar 14, 2018

For what it's worth I haven't been able to reproduce the recursion error locally either, but I have seen it on Travis (and other CI systems). So I'm fairly confident there is a real issue somewhere in our tests or one of our dependencies. Most likely there's a rogue mock/patch on stdout messing everything up.

@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

Well, Travis also does not like this one - will close/re-open.

@blueyed blueyed closed this Mar 14, 2018
@blueyed blueyed reopened this Mar 14, 2018
@blueyed
Copy link
Author

blueyed commented Mar 14, 2018

Maybe #1408 helps in this regard - it is unnecessary to wrap running the tests with shell=True.

@joguSD
Copy link
Contributor

joguSD commented Mar 14, 2018

I think there's just quite a few branches / PRs queued up on travis that it's taking a while to get to these new ones. Just give it some time and I think they'll run.

@blueyed
Copy link
Author

blueyed commented Mar 15, 2018

@manselmi
Copy link

+1 to relaxing the python-dateutil version restriction for Python 2.7+ (this was breaking our internal builds). Thanks for the PR @blueyed!

@prometheanfire
Copy link
Contributor

yes, please relax this, openstack is not able to update dateutil because of this.

http://logs.openstack.org/06/552806/4/check/requirements-tox-py27-check-uc/4d47d21/job-output.txt.gz#_2018-03-16_15_35_57_233537

2018-03-16 15:35:57.233537 | ubuntu-xenial | ContextualVersionConflict: (python-dateutil 2.7.0 (/home/zuul/src/git.openstack.org/openstack/requirements/.tox/py27-check-uc/lib/python2.7/site-packages), Requirement.parse('python-dateutil<2.7.0,>=2.1'), set(['botocore']))

@ryanpetrello
Copy link

ryanpetrello commented Mar 16, 2018

👍, this is also preventing the Ansible AWX project from using the latest boto and the latest python-dateutil together:

ansible/awx#1598

'docutils>=0.10']

if sys.version_info <= (2, 7):

Choose a reason for hiding this comment

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

I think this switch only works depending on what version of Python you run setup.py with, not the version that you're installing this for.

Shouldn't this be something more like:

requires.extend(["python-dateutil>=2.1,<2.7.0;python_version == '2.6'",
                           "python-dateutil>=2.1,<3.0.0;python_version >= '2.7'"])

That way it will also work if you build a wheel on Python 3.6 and install it with python 2.6.

Copy link
Author

Choose a reason for hiding this comment

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

@pganssle
Good call.
But sys.version_info is also used below, which should be changed then, too.
IIRC this might require a certain version of setuptools etc (I think checking pytest's setup.py might be good).

Choose a reason for hiding this comment

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

At dateutil we ended up doing this to ensure a minimum requirement, but it might be possible to use install_requires.

@prometheanfire
Copy link
Contributor

anything left to do here before merge?

openstack-gerrit pushed a commit to openstack/requirements that referenced this pull request Mar 30, 2018
Removed:
 - pecan :: https://review.openstack.org/555891
 - pbr :: https://review.openstack.org/557009
 - pika
 - Sphinx
 - oslo.config :: https://review.openstack.org/557012
 - pyldap :: frankly it looked strange and I wanted to investigate
 - botocore :: boto/botocore#1406

Change-Id: Ibddac92fb339b3678a231c4e939e62aba1e15f08
@pganssle
Copy link

@prometheanfire I think the sys.version_info stuff needs to be fixed, this will only really work correctly for sdist installations.

@pganssle
Copy link

That said, I'm still dubious as to whether this is necessary at all. It seems like a belt-and-suspenders approach. Updating pip will solve the problem in the vast majority of cases. Some people on old versions of Artifactory or other mirrors that don't respect Requires-Python may also be affected.

@prometheanfire
Copy link
Contributor

hmm, ya, just fixing requirements.txt to be the following should work

python-dateutil>=2.1,<2.7.0;python_version=='2.6'
python-dateutil>=2.1,<3.0.0;python_version>='2.7'

as an example, see https://github.com/openstack/requirements/blob/master/global-requirements.txt#L75

@pganssle
Copy link

pganssle commented Mar 30, 2018

@prometheanfire What I'm saying that leaving the requirements.txt as:

python-dateutil>=2.1,<3.0.0

Will also work, because python-dateutil >= 2.7.0 has this line in its setup.py, so anyone doing pip install botocore on Python 2.6, so long as they are using pip >= 8.0, will already get python-dateutil~=2.6.

That said, your way is a "belt and suspenders" approach, in that it will also work for people using very old versions of pip.

I generally think, "update pip" is a reasonable thing to ask at this point, given that as more people drop compatibility for Python 2, Requires-Python is going to be critical. It's already a total pain to try and do things like test against Python 3.2, because pip 8.0 is not supported on that platform, and pytest and a bunch of its transitive dependencies have dropped support for Python 3.2. That situation is only going to get worse.

@prometheanfire
Copy link
Contributor

True, I'd be happy with either method. We (gentoo) have pip 7.1.2 available, but pip 9 has been stable for a long time. The current situation is preventing OpenStack from consuming the latest botocore, so solving it in some way would be nice (and the requisite release). I can submit another PR to do the requirements update unless someone's going to update this one.

@jakul
Copy link

jakul commented Apr 10, 2018

What needs to be done to get this merged?

@blueyed
Copy link
Author

blueyed commented Apr 10, 2018

@jakul
I think this should not get merged as-is, see the previous comment (#1406 (comment)).
It's up to maintainers though, so let's just wait for them I guess?!

@blueyed
Copy link
Author

blueyed commented Apr 10, 2018

Closing.
Maintainers should be aware, and otherwise an issue should be created instead probably.
While this PR would work, #1406 (comment) seems to be a better approach.
I could update the PR accordingly, but it's as easy for maintainers to do so directly probably.
This PR also already contains too much noise.

@hugovk
Copy link
Contributor

hugovk commented Sep 8, 2018

I generally think, "update pip" is a reasonable thing to ask at this point, given that as more people drop compatibility for Python 2, Requires-Python is going to be critical.

Agreed, see #1415 or #1434 to add Requires-Python support in botocore.

@blueyed blueyed deleted the dateutil branch September 8, 2018 14:57
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.

None yet

9 participants