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

revert PR9; add --no-deps to pip wheel; address PR11 #18

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

tgarc
Copy link
Contributor

@tgarc tgarc commented Jul 9, 2017

This is a WIP - there will be some tweaking but the proof of concept is there and should be fully functional. I've tested this with my package on all platforms. This should address the issue raised by PR #11 without needed an additional pip pre-install step. It also removes the step of building wheels for dependencies - a step which adds extra build time and may even cause issues for some packages. BTW this follows more closely the build steps in the https://github.com/matthew-brett/multibuild project.

Revert PR9; it was never actually needed in the first place it just fixed an
issue by side effect.

  1. add --no-deps to pip wheel step in order to avoid building wheels for
    dependencies. This also addresses the issues raised in PR Solving problems in OS X when library has dependencies #11.

  2. Modify the pip install step across platforms to allow installation of
    dependencies from the index. (This is required since we will no longer be
    building wheels for dependencies).

@tgarc
Copy link
Contributor Author

tgarc commented Jul 9, 2017

@YannickJadoul would you test this?

@YannickJadoul
Copy link
Member

YannickJadoul commented Jul 9, 2017

@tgarc Yes, please! Thanks. I'll let you know.

If you want, I'd be curious to have your input on the solution to the other issues in PR #17, too.

EDIT: If you want to follow live, here's my build on Travis CI :-)
https://travis-ci.org/YannickJadoul/Parselmouth/jobs/251766087

@YannickJadoul
Copy link
Member

Seems to be working! :-)

2 questions, if I may:

  1. So now the install step before is not happening, I see. So how are the dependencies installed (the ones that PR add a 'pip install' step before building wheels #9 adressed, such as that 'cffi' you needed) ?

  2. Why is the filter_wheels needed? Can't you just pip install -f /output and let pip sort out which versions it can use?

@tgarc
Copy link
Contributor Author

tgarc commented Jul 9, 2017

@YannickJadoul thanks for testing!

So how are the dependencies installed (the ones that PR #9 adressed, such as that 'cffi' you needed) ?

Unfortunately I caused some confusion with my previous PR. Dependencies aren't usually required to do a wheel build, only to install that wheel. (Although some builds do require 'build-dependencies'). In my case doing a pre- pip install was fixing an issue by side-effect that was actually being caused by including setup_requires=['cffi']. (This seems like a bug in cffi or setuptools etc). IOW, my PR was only ever a workaround for a cffi issue.

Why is the filter_wheels needed? Can't you just pip install -f /output and let pip sort out which versions it can use?

Okay so now that no wheels are being built for dependencies I had to remove the --no-index so that pip can search PyPI for packages. Unfortunately, pip has no way to specify preference with the --find-links option so that it installs the local wheel instead of the package found on PyPI. Instead it will just install the listed packages from the first place that it can find it (which happens to be PyPI). Does that make sense? This same issue was actually brought up on the pip issue tracker on github.

@tgarc tgarc force-pushed the revert_pr9 branch 2 times, most recently from 8e22fef to 7a2e6a2 Compare July 9, 2017 20:48
@tgarc
Copy link
Contributor Author

tgarc commented Jul 9, 2017

Okay, think I've cleaned up as much as I can. @joerick take a look when you have time.

@YannickJadoul
Copy link
Member

Unfortunately I caused some confusion with my previous PR. Dependencies aren't usually required to do a wheel build, only to install that wheel. [...]

Ok, perfect :-) That makes things easier than I expected than, indeed :-)

[...] Instead it will just install the listed packages from the first place that it can find it (which happens to be PyPI). Does that make sense? [...]

Yeps, that does. Weirdly enough though, I had tried that myself, before, and had even been looking up in the pip source (https://github.com/pypa/pip/blame/21be153044a7aa245e12ce3f86793e9b17201519/pip/index.py#L464-L468) and thought that local files díd get precedence. But I was just curious. This is not a monster hack; it's pretty neat, actually. Just a pity you nééded a workaround.

@YannickJadoul
Copy link
Member

Aaaaaaaaaah, I've actually just been testing (with -vvv, extra extra verbose pip), and I think I just realized why the tests are failing and why you need that filter_wheels command: I think pip dóes give precedence to local files, but it gives even more precedence to having the latest version.

In this case, there is already a spam package in the PyPI (https://pypi.python.org/pypi/spam), meaning that even though the local spam wheel (with -f) would be preferred to the PyPI, the PyPI one has a higher version (4.0.9) which shadows the wheel that was just built (0.1.0).

@@ -17,11 +17,11 @@ def build(project_dir, package_name, output_dir, test_command, test_requires, be
request = urlopen('https://github.com/ogrisel/python-appveyor-demo/raw/09a1c8672e5015a74d8f69d07add6ee803c176ec/appveyor/run_with_env.cmd')
f.write(request.read())

def shell(args, env=None, cwd=None):
def shell(args, env=None, cwd=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

kwargs does not seem to be used anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Thanks.


temp_wheel = glob(temp_wheel_dir+'/*.whl')[0]
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance that multiple wheels get built?
I don't see how or why one package could do that, but the Linux version is looping over all wheels. So for consistency we can maybe either change this line, or the one in linux.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still an open question. I still don't know of any projects where multiple wheels get built so I decided not to make any changes, at least for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fair enough. Fine with me.
I have just realised that "for whl in /tmp/linux_wheels/*.whl; do" is probably just easier to write the pattern match, and not per se to do that over all wheels.

Copy link
Contributor

@joerick joerick Jul 13, 2017

Choose a reason for hiding this comment

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

I've not seen any packages that make more than one wheel, so I think this is a good assumption 👍

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Thanks tgarc! I think maybe removing the find_wheels bits and going back the previous install command would simplify this a bit?

sys.stdout.write(\
' '.join(f for f in itertools.chain(*map(glob.iglob, sys.argv[1:])) \
if pip.wheel.Wheel(f).supported()))\
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm finding this quite hard to understand. What does this do, and might there be a more elegant way to do it? I'm not averse to separate code for each platform by the way!

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think this is an undocumented Pip API... Might change in the future.

Copy link
Contributor Author

@tgarc tgarc Jul 11, 2017

Choose a reason for hiding this comment

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

Sorry, lack of commenting. I can add some.
This is a short python program that takes as arguments a list of wheels and prints out only those wheels that are supported on the executor's platform. I put it here since all platforms use it and there's no need to have it differ per-platform.

But if you're asking why...hopefully my previous comment makes it clear.

Also I think this is an undocumented Pip API... Might change in the future.

I will try and verify whether that's the case. I tend to assume anything not starting with an underscore is fairly stable but this may not be true in pip world.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I had missed that previous comment! Yes I think you're right in that respect.

@@ -77,8 +74,7 @@ def build(project_dir, package_name, output_dir, test_command, test_requires, be
# Install packages and test
for PYBIN in {pybin_paths}; do
# Install the wheel we just built
"$PYBIN/pip" install {package_name} \
--upgrade --force-reinstall --no-deps --no-index -f /output
"$PYBIN/pip" install $("$PYBIN/python" -c "{filter_wheels}" /tmp/linux_wheels/*.whl)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change? I think the previous one is clearer and also it installs the delocated wheel, which is probably the right thing to do!

Copy link
Contributor Author

@tgarc tgarc Jul 11, 2017

Choose a reason for hiding this comment

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

Yikes! you're absolutely right, I meant to point to the delocated wheel. See my previous comment for why I made this change though.
EDIT fixed; correctly points to delocated wheel now

@tgarc
Copy link
Contributor Author

tgarc commented Jul 11, 2017

@YannickJadoul

In this case, there is already a spam package in the PyPI (https://pypi.python.org/pypi/spam), meaning that even though the local spam wheel (with -f) would be preferred to the PyPI, the PyPI one has a higher version (4.0.9) which shadows the wheel that was just built (0.1.0).

That's accurate. In fact it says this in the pip docs:

[...] [pip] chooses which version of each requirement to install using the simple rule that the latest version that satisfies the given constraints will be installed

@joerick
Copy link
Contributor

joerick commented Jul 13, 2017

I got a chance to look at this again - I'm wondering if a simpler solution to the filter_wheels business would be to delocate/audit into a directory made by tempfile.mkdtemp(), and then copy into output_dir after the tests have run - that way we can tell pip to install exactly the wheel we want, and we can get the path of that using a single glob.glob call.

I'm sorry to be slowing things down! I just think that it's pretty crucial for the project to try and keep this code simple so that people can peek under the hood when things go wrong and see how it works :)

@tgarc
Copy link
Contributor Author

tgarc commented Jul 14, 2017

I'm sorry to be slowing things down!

No worries, I'm busy too. Thanks for sharing your code.

So use a temp directory for installing...interesting. I think I can do that. I still think that the filter_wheels functionality may come in handy at some point but if we can do it your way...sounds cleaner.

EDIT

Ah, I forgot linux does things differently - it builds all the wheels before installing any of them. I'll have to add some bash code to make this work then.

@tgarc
Copy link
Contributor Author

tgarc commented Jul 15, 2017

@joerick here's a shot at doing what you suggested. Note one significant change: the linux build now more closely follows builds on macos/windows; it uses a single for loop to build/delocate/install/test each wheel. One thing about this that irks me a bit is that if ever pip wheel did produce more than one wheel the linux build would produce some cryptic error whereas other platforms only select the first globbed wheel.

Anyway, the tests are failing and I have only tested with windows so I'll need to fix this up.

@tgarc
Copy link
Contributor Author

tgarc commented Jul 15, 2017

bah. this has turned out to be more work than anticipated. seems like for some reason the last step (shutil.move(temp_wheel, output)) is failing on windows & macos. Feel free to chime in if you have thoughts. @joerick is this what you envisioned? Does it seem simpler to you ?

@YannickJadoul
Copy link
Member

@tgarc Just to debug, could you maybe shell(['ls', temp_wheel_dir], env=env); shell(['ls', output_dir], env=env) before and after that shutil.move and maybe also print the values of the dir?
I'd do this myself, but since you have this PR, letting the CI run seems easier :-)

@tgarc tgarc force-pushed the revert_pr9 branch 2 times, most recently from 883802d to 6e76dc8 Compare July 18, 2017 04:00
@tgarc
Copy link
Contributor Author

tgarc commented Jul 18, 2017

@YannickJadoul found the bug...I needed to add a step to create the output directory. Previously the output directory was getting automatically created by one of the pip commands

@tgarc
Copy link
Contributor Author

tgarc commented Jul 18, 2017

@joerick okay I've found the bugs that I had. The TL;DR is that I had to create extra temp directories for delocated wheels, and manually delete any temporary wheels for each build so they don't get picked up on subsequent builds. The complexity I think is about the same as with filter_wheels but I think the code 'looks' simpler. Would you give it a look over for sanity? Then I'll squash the commits and merge it.

@YannickJadoul
Copy link
Member

@tgarc Nice! My test build is also succeeding: https://travis-ci.org/YannickJadoul/Parselmouth/jobs/251766087

@joerick
Copy link
Contributor

joerick commented Jul 18, 2017

Sweet! I'm taking a look now.

@joerick
Copy link
Contributor

joerick commented Jul 18, 2017

Hey @tgarc - this looks good, I just did a few naming tweaks and changes to the temp dir stuff, hopefully that makes the code clearer. Could you pull from my revert_pr9 branch? Then as long as you're happy, I'm cool to merge!

@tgarc tgarc force-pushed the revert_pr9 branch 2 times, most recently from 741d0cd to 638d854 Compare July 19, 2017 02:44
@tgarc
Copy link
Contributor Author

tgarc commented Jul 19, 2017

thanks @joerick
squashed and rebased. I'll wait until I'm a little more clear headed to give it a last once over before merging.

revert PR9; add --no-deps to pip wheel

Revert PR9; it was never actually needed in the first place it just fixed an
issue by side effect.

1) add --no-deps to pip wheel step in order to avoid building wheels for
   dependencies. This also addresses the issues raised in PR pypa#11.

2) Modify the pip install step across platforms to allow installation of
   dependencies from the index. (This is required since we will no longer be
   building wheels for dependencies).
@tgarc tgarc merged commit f26da27 into pypa:master Jul 20, 2017
@joerick
Copy link
Contributor

joerick commented Jul 20, 2017

🎉

@joerick
Copy link
Contributor

joerick commented Jul 23, 2017

just released 0.4.0 with these changes. 👍

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