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

MAINT: Fix wheel build to ensure futures is only required in Python 2 #2678

Merged
merged 7 commits into from
Aug 10, 2018

Conversation

effigies
Copy link
Member

Summary

conda create -y -n test python=3.6
source activate test
pip install nipype==1.1.1
pip show futures

Output:

Name: futures
Version: 3.1.1
Summary: Backport of the concurrent.futures package from Python 3.2
Home-page: https://github.com/agronholm/pythonfutures
Author: Brian Quinlan
Author-email: brian@sweetapp.com
License: PSF
Location: /anaconda3/envs/test/lib/python3.6/site-packages
Requires: 
Required-by: nipype

This PR will attempt to ensure the wheel is correctly created to only install futures in Python 2. My suspicion is that upgrading setuptools before building the wheel is the correct fix.

I suggest that we add a hot-fix release, so that downstream Python 3 packages can stop installing futures ASAP.

List of changes proposed in this PR (pull-request)

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@effigies effigies added this to the 1.1.2 milestone Aug 10, 2018
@effigies
Copy link
Member Author

That resolved it:

$ grep futures nipype-1.1.*/*
nipype-1.1.1.dist-info/METADATA:Requires-Dist: futures
nipype-1.1.2.dev0+g8f744ec33.dist-info/METADATA:Requires-Dist: futures; python_version == "2.7"
nipype-1.1.2.dev0+gae913a2b2.dist-info/METADATA:Requires-Dist: futures

And pip installing nipype-1.1.2.dev0+g8f744ec33-py2.py3-none-any.whl installs futures on Python 2, but not Python 3.

I need to update the tag build, but I'm going to see if I can add some install tests to the PyPI test so we can be sure this doesn't crop back up.

@effigies
Copy link
Member Author

Okay, ready for review. @mgxd @satra How do you feel about a hotfix? And should we do it off of master (with some post-1.1.1 changes already in), or just these changes on top of 1.1.1?

Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

LGTM, i'm fine with releasing 1.1.2 a little early to get this fix out

@@ -393,6 +409,7 @@ workflows:
version: 2
build_test_deploy:
jobs:
- pypi_precheck
Copy link
Member

Choose a reason for hiding this comment

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

is this running on every build or just tagged ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Every build. The idea is to get an error before we tag and have to re-tag. I was only doing it on rel/* branches, but it's a 2min job.

Copy link
Member

Choose a reason for hiding this comment

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

I like doing it on rel/ branches since we'll tag after those pass - but 2 mins is not the end of the world for me

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll move it back in the rel branch.

@effigies effigies changed the title MAINT: Only require futures in Python 2 MAINT: Fix wheel build to ensure futures is only required in Python 2 Aug 10, 2018
@effigies
Copy link
Member Author

Do we want to do 1.1.3 at the end of the month, the end of next month, or shoot for a month from 1.1.2?

@mgxd
Copy link
Member

mgxd commented Aug 10, 2018

i vote targeting end of next

@effigies
Copy link
Member Author

Sounds good. Created milestone 1.1.3 and moved everything else there. I'll merge on pass and start a rel/1.1.2 branch.

@satra
Copy link
Member

satra commented Aug 10, 2018

@effigies - hotfix is good to release.

@effigies effigies merged commit 297a24c into nipy:master Aug 10, 2018
@effigies effigies deleted the maint/wheels branch August 10, 2018 21:41
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.

3 participants