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

Allow use with docutils >=0.16 for Python 2.7 and 3.5+ #2011

Closed
wants to merge 1 commit into from
Closed

Allow use with docutils >=0.16 for Python 2.7 and 3.5+ #2011

wants to merge 1 commit into from

Conversation

yan12125
Copy link

@yan12125 yan12125 commented Apr 5, 2020

For #1942. Rebased from #1944.

@yan12125
Copy link
Author

yan12125 commented Apr 5, 2020

test_stress_test_token_bucket appears to be flaky. It fails from time to time [1][2][3].

[1] https://travis-ci.org/github/boto/botocore/jobs/667356837
[2] https://travis-ci.org/github/boto/botocore/jobs/665387607
[3] https://travis-ci.org/github/boto/botocore/jobs/655670871

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2020

Codecov Report

Merging #2011 into develop will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2011   +/-   ##
========================================
  Coverage    93.10%   93.11%           
========================================
  Files           60       60           
  Lines        11096    11096           
========================================
+ Hits         10331    10332    +1     
+ Misses         765      764    -1     
Impacted Files Coverage Δ
botocore/credentials.py 98.64% <0.00%> (+0.10%) ⬆️

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 d46c8c5...00b14dc. Read the comment docs.

@PrimozGodec
Copy link

Is there a plan for this PR to be merged soon. What is the reason for the wait?

@hrw
Copy link

hrw commented Jun 10, 2020

I do wonder what is a point of switch to "<1.0.0" when 0.16 is latest.

Maybe just leave at ">0.10" instead?

@PrimozGodec
Copy link

@hrw I really agree with you. Same should be done with python-dateutil

@yan12125
Copy link
Author

After checking more details, the situation is more complex than I thought. As botocore still supports Python 3.4, and docutils 0.16 no longer supports Python 3.4, I have to use branched requirements. Anyway, in the branch for Python != 3.4, I removed the upper bound for docutils.

For python-dateutil, I think it belongs to a separate pull request. I'm not sure how AWS staffs think, though.

@yan12125 yan12125 changed the title setup.py: Allow use with docutils >=0.16 setup.py: Allow use with docutils >=0.16 for Python 2.7 and 3.5+ Jun 10, 2020
'python-dateutil>=2.1,<3.0.0',
]


if sys.version_info[:2] == (3, 4):
# urllib3 dropped support for python 3.4 in point release 1.25.8
requires.append('urllib3>=1.20,<1.25.8')
# docutils dropped support for python 3.4 in release 0.16
requires.append('docutils>=0.10,<0.16')
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be part of the requires set above.

docutils>=0.10,<0.16;python_version=='3.4'

and the line above should be as follows

docutils>=0.10;python_version>='3.5'

Copy link
Contributor

@prometheanfire prometheanfire Jun 10, 2020

Choose a reason for hiding this comment

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

You can see some examples of what is done elsewhere in

https://github.com/openstack/requirements/blob/master/global-requirements.txt

debtcollector is a good example right now

Copy link
Author

Choose a reason for hiding this comment

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

Thanks I know that syntax. I put requirements in if/else after seeing a comment from 2018 against using environment markers in setup.py [1].

@joguSD Are environment markers in setup.py still discouraged?

[1] #1433 (comment)

@yan12125
Copy link
Author

Ouch, I forgot to update requirements.txt and other relevant files. Now they are updated, too, and Travis CI is still green with docutils 0.16 installed on 2.7 and 3.5+ builders.

@yan12125 yan12125 changed the title setup.py: Allow use with docutils >=0.16 for Python 2.7 and 3.5+ Allow use with docutils >=0.16 for Python 2.7 and 3.5+ Jun 10, 2020
Keep docutils < 0.16 on Python 3.4 as docutils 0.16 no longer supports
Python 3.4.

See: https://docutils.sourceforge.io/RELEASE-NOTES.html#release-0-16-2020-01-12
@yan12125
Copy link
Author

After reading all issues around docutils, I believe the long term solution should be remove docutils from botocore to aws-cli as described in [1][2]. I will not work on this pull request anymore. If anyone is still interested in bumping docutils version in botocore, feel free to take my patch and open another pull request.

[1] #900
[2] aws/aws-cli#4719

@yan12125 yan12125 closed this Jun 11, 2020
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.

6 participants