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

Solving problems in OS X when library has dependencies #11

Closed
wants to merge 1 commit into from

Conversation

YannickJadoul
Copy link
Member

@YannickJadoul YannickJadoul commented Jun 11, 2017

I ran into problems when I tried using cibuildwheel on Travis CI's OS X, because my library depends on another wheel from the PyPI (numpy, in this case). (In particular in the final installstep, since it could not find the wheel of my library it had just built.)

Looking into the code, I figured the problem was the line that says temp_wheel = glob(temp_wheel_dir+'/*.whl')[0] and thus basically ignores that pip wheel . will by default download the wheels on which the library depends.

This PR simply solves that by not downloading those dependencies until the actual install step. However, I'm not sure this is the very best solution and I am not an expert on setuptools and pip, so maybe there are other possibilities to consider... ?

@joerick
Copy link
Contributor

joerick commented Jun 11, 2017

Hey @YannickJadoul - thanks for doing a bit of debugging! We've been looking at some similar things over in #9 - perhaps you could give the latest release 0.2.1 a shot and see if that fixes things?

@YannickJadoul
Copy link
Member Author

Oh, sorry, I should've checked the other pull requests better, before. But building on the new release again, so let's see: https://travis-ci.org/YannickJadoul/Parselmouth/builds/241758708

@YannickJadoul
Copy link
Member Author

My build is timing out now, because for some reason, the whole library is fully compiled twice (the first time after python setup.py install, and a second time for pip wheel). Since my library is quite big, or feels as if this solution is not fully satisfactory for me..

@tgarc
Copy link
Contributor

tgarc commented Jun 11, 2017

@YannickJadoul Good catch! I hadn't realized the glob(...)[0]. That seems like a bug given that it is not the behavior for other platforms. To be consistent with the linux build, this should really be for whl in glob(...): ....

As for my recent PR and your issue with it: I hadn't imagined the double compilation would be an issue but now I see that it is...I think ideally you would want to do a pip install -r requirements.txt before doing the wheel build; however, not everybody has/needs a requirements file hence why I did a pip install instead. If there was a way to do pip install --dependencies-only that would be ideal. It seems like this could be done with pip freeze

@joerick
Copy link
Contributor

joerick commented Jun 11, 2017

@YannickJadoul gah, sorry, that's annoying. Maybe you could use CIBW_SKIP in a Travis matrix to split your builds onto two machines so you stay under the limit?

@tgarc agreed re for whl in glob(...):. I also didn't think the double-compilation would be a large problem, but I guess it really is for some large projects. Let's have a think if there's a way we can get around this...

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jun 12, 2017

gah, sorry, that's annoying. Maybe you could use CIBW_SKIP in a Travis matrix to split your builds onto two machines so you stay under the limit?

Hmmm, that's maybe also a plan, yes. I was also thinking of setting up ccache (since most of the project can be compiled without Python-specific things)

I also didn't think the double-compilation would be a large problem, but I guess it really is for some large projects.

So, with the ccache I could probably get around it, but the perfectionist in me just conceptually doesn't like that that compilation is triggered twice. Also (ignore my ignorance), but what exactly is the difference between pip wheel and python setup.py bdist_wheel?

To be consistent with the linux build, this should really be for whl in glob(...): ...

I'm all for consistency, but why should that be a for-loop, actually? After all, the dependencies are separate projects (probably on PyPI), so you don't need to delocate/auditwheel all of them, do you?

@YannickJadoul
Copy link
Member Author

I've started looking at this problem again, seeing if there's some kind of flag for pip wheel or setup.py install to solve this double compilation.

But in the meanwhile: could someone please explain again what the actual purpose of PR #9 was?
(I can see what it does, but I cannot figure out what the original problem was it was trying to solve.)

@joerick
Copy link
Contributor

joerick commented Jun 28, 2017

#9 was solving the problem in #6. Some packages need their dependencies installed before they will compile. This is described in setup.py install_requires, but pip does not install dependencies before trying to build the wheel. So instead we install first, which installs the deps, then we build the wheel.

I think we need a way of getting the dependencies from a package, so we can pip install them, and then do a pip wheel of the target package. This should be possible using python setup.py --requires, but I can't get that to work - maybe I'm missing something. If we can figure out how pip finds dependencies and use that, that may be the best route.

@YannickJadoul
Copy link
Member Author

Ah, I see, I'd looked over those details; thanks!

Just playing the devil's advocate: is that the actual function of 'install_requires', actually? The Python documentation says install_requires is a setuptools setup.py keyword that should be used to specify what a project minimally needs to run correctly. When the project is installed by pip, this is the specification that is used to install its dependencies.
So which makes it more seem like a problem with setup.py and the way it handles 'compile-time dependencies' (vs. 'run-time dependencies'), rather than a cibuildwheel problem? Or am I talking nonsense, here?

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jun 28, 2017

Just tested, and python setup.py --requires seems to react to the setup_requires field in the call to setup (i.e., I put setup_requires = ["cffi"], and Python downloads that to a cached folder .eggs and manages to import cffi from the build script (even though it's not really installed and I cannot import it plainly from Python))

Edit: finally found sóme documentation on setup_requires: https://setuptools.readthedocs.io/en/latest/setuptools.html#new-and-changed-setup-keywords

@joerick
Copy link
Contributor

joerick commented Jun 28, 2017

I think you're right @YannickJadoul re. install_requires vs. setup_requires, however, the distinction is not really seen elsewhere in the packaging ecosystem so most devs don't know about setup_requires. I've had a couple tickets about this already on this repo, so I'm thinking it's better just to sidestep the problem and install dependencies before trying to build.

I also think I had one ticket that wasn't solved by the setup_requires. Ah, yes I found it. #2 (comment) The problem being that the non-standard .eggs installation means the header files are in a different place. Ah, packaging 😉.

I guess if we can't figure out the double-compile problem we could add an option to disable the 'pip install' step, but keeping the slower and safer way by default.

But I'm still hopeful about installing the deps before!

@tgarc
Copy link
Contributor

tgarc commented Jun 28, 2017

I think we need a way of getting the dependencies from a package, so we can pip install them, and then do a pip wheel of the target package.

I mentioned this in passing earlier, but pip freeze seems to automatically generate a requirements file for a package so that may be a potential solution.

I was planning at some point to look a little closer at why pip install is really required in my case (#6) and if there was some other way around it but I haven't found the time just yet.

@YannickJadoul
Copy link
Member Author

@joerick Yeah, I was still just trying to understand everything, not trying to make a point on how I think things should be done. After all, the whole packaging procedure ís a complex mess, which is why I was so happy to find your small but oh-so-useful library ;-)

I can probably 'fix' the double compilation by using ccache (so it will just preprocess and see it can use the cached object files), but that still feels as if there should be a better option.
But if I find the time, somewhere, I'll also have a look at what I can come up with and update this PR.

@joerick
Copy link
Contributor

joerick commented Jun 29, 2017

@tgarc I can't see how pip freeze would print the deps without already having done pip install first? (we're trying to avoid the initial pip install step)

@YannickJadoul
Copy link
Member Author

I actually found this thread on StackOverflow, yesterday: https://stackoverflow.com/questions/24236266/how-to-extract-dependencies-information-from-a-setup-py
But it feels as if there ought to be a less hackish way of retrieving those dependencies (maybe implementing a custom setup.py target or so)...
Plus, this code has a dependency on mock, though I guess cibuildwheel could depend on mock if we'd go for using that.

@tgarc
Copy link
Contributor

tgarc commented Jun 29, 2017

Joe, my mistake, I didn't realize pip freeze only works for installed packages. Although with some further searching I came across pip-compile : seems painless enough. I wonder if that might do the trick.

@joerick
Copy link
Contributor

joerick commented Jun 29, 2017

Aha! I did a little trawling through pip-tools and found distutils.core.run_setup(). That looks to do what we want :)

joerick@joerick ~/04-Open-source-projects/pyinstrument> python
Python 2.7.10 (default, Jul 30 2016, 18:31:42) 
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import distutils
>>> import distutils.core
>>> distribution = distutils.core.run_setup('setup.py')
>>> distribution.install_requires
['pyinstrument_cext']

@joerick
Copy link
Contributor

joerick commented Jun 29, 2017

Also, I had a look at pip - found that it seems to use pkg_resources from setuptools

>>> import pkg_resources
>>> distribution = pkg_resources.find_distributions('.', only=True).next()
>>> distribution.requires()
[Requirement.parse('pyinstrument_cext')]

@tgarc
Copy link
Contributor

tgarc commented Jun 30, 2017

@YannickJadoul
I want to understand you're original problem a little better. Am I right in assuming you have a setup.py and numpy is listed under the install_requires dependencies? If so, when using cibuildwheel == 0.2.0 are you experiencing the problem only on mac osx and not on other platforms? And that problem is that pip wheel . is not installing numpy?

I ask because for my package (github.com/tgarc/pastream) pip wheel . does seem to install dependencies for several environments but fails to in others, for example, python 3.4 on linux. So I'm trying to get a better understanding of the problem before committing a workaround that will screw something else up in the future.

@YannickJadoul
Copy link
Member Author

@tgarc The original issue of this PR is that pip wheel . did not just build a wheel, but also downloads the other wheels from PyPI that are in the install_requires list (indeed, numpy in my particular case). Because of this, temp_wheel = glob(temp_wheel_dir+'/*.whl')[0] selects an arbitrary wheel to fix up and delocate (or auditwheel, but I haven't set up things on Linux yet) instead of mý library.

But your pull request does fix that, actually, because you now run the pip wheel . with --no-deps.

So my current problem is that my C code base is rather big (since I'm wrapping an existing program) and takes a long time to compile. Since you added the python setup.py install step before the pip wheel in your PR (and I now understand why), all C code is compiled twice.
And that makes Travis CI time out. I know that there's ways around that (as @joerick suggested), but it just feels wrong that everything needs to be built twice.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jun 30, 2017

There's this package, extending setuptools: https://setupext-pip.readthedocs.io/en/latest/requirements-command.html
But it also only uses the setup_requires (and mentions the difference: https://setupext-pip.readthedocs.io/en/latest/requirements-files.html#using-requirement-files)

But if we want, I suppose we could do something similar?

Edit: The other option is to point people to setup_requires but use this setupext-pip project to install those packages (fixing the kinds of errors as you referred to, #2, since they will be installed via pip and wheels instead of eggs).

@tgarc
Copy link
Contributor

tgarc commented Jun 30, 2017 via email

@tgarc
Copy link
Contributor

tgarc commented Jul 1, 2017

@joerick

I'm thinking maybe we should revert #9. Instead, what about this:

  1. Add a CIBW_REQUIREMENTS_FILE environment variable using requirements.(txt|in) as the default. If that file exists, do a pip install -r <requirements> before running pip wheel.
  2. If we figure out a way to generate a requirements file reliably, we can use this as a default if CIBW_REQUIREMENTS_FILE is not set
  3. Replace temp_wheel = glob(temp_wheel_dir+'/*.whl') with for whl in glob(temp_wheel_dir+'/*.whl'): to fix the issue @YannickJadoul brought up.

This is just a suggestion so please free to criticize, but it seems like pre-installing a requirements file is a common build step (I'm mostly following the manylinux example).

BTW, do you have any ideas on why users would need to retrieve wheels for all their dependencies (as is automatically done when running pip wheel)? I think this may be part of deployment processes which are more complicated than I've ever had experience with but it would probably be useful to know something about this requirement.

@joerick
Copy link
Contributor

joerick commented Jul 2, 2017

Hey @tgarc. Wow, thanks for the thinking you've done here.

Add a CIBW_REQUIREMENTS_FILE environment variable using requirements.(txt|in) as the default. If that file exists, do a pip install -r before running pip wheel.

I'm going to push back a little on using requirements.txt - in the case where the wheel doesn't exist, pip will download and build a module from the source tarball. Therefore the requirements to build a package should always be listed in setup.py. Some modules do also have a requirements.txt, but it normally it contains development utils, and -e ., which will install the module and deps as listed in setup.py.

If we figure out a way to generate a requirements file reliably, we can use this as a default if CIBW_REQUIREMENTS_FILE is not set

Agreed, this is the best approach. Perhaps using the distutils command I listed above.

Replace temp_wheel = glob(temp_wheel_dir+'/.whl') with for whl in glob(temp_wheel_dir+'/.whl'): to fix the issue @YannickJadoul brought up.

BTW, do you have any ideas on why users would need to retrieve wheels for all their dependencies

No, I don't acutually. I think it was just an interesting side-effect of the code as written that multiple wheels would get produced.


I got the distutils command down to a one-liner, might be useful for the linux build

python -c 'import distutils.core; print " ".join(distutils.core.run_setup("setup.py").install_requires or [])' 

@YannickJadoul
Copy link
Member Author

Cool, this indeed seems like a good path to take to me. If we can either make the installing of depencencies a no-op in case nothing need to be done, or an optional thing, users could do what seems best to them.

No, I don't acutually. I think it was just an interesting side-effect of the code as written that multiple wheels would get produced.

Is that possible, to build multiple wheels in one package? Either way, as long as we keep the --no-deps flag, pip wheel won't start downloading wheels that don't need to be delocated/auditwheeled, so the for won't hurt there.

I got the distutils command down to a one-liner, might be useful for the linux build

Nicely done, looks clean. But is distutils still supported by Python? I thought setuptools took over that function? (But I'm not an expert on Python packaging, so maybe that's wrong?)
@joerick What do you think of the other option, to not generate a requirements file, but to write a small extension to setuptools that would allow for python setup.py install_requirements or so?

@tgarc
Copy link
Contributor

tgarc commented Jul 2, 2017

I'm going to push back a little on using requirements.txt -

Yeah, sounds reasonable. With a little more thought, I don't think there's any apparent reason why we would want to include this as default behavior. I suppose people can always install using BEFORE_BUILD if needed.

do you have any ideas on why users would need to retrieve wheels for all their dependencies

No, I don't acutually. I think it was just an interesting side-effect of the code as written that multiple wheels would get produced.

I just wonder whether this side effect is benign or not. From what I've read and seen in other builds, it seems like --no-deps should probably be the default.

BTW, just in case you haven't seen this before, there is another project for wheel building available which quite a bit more complex: https://github.com/matthew-brett/multibuild . Obviously, I prefer the simplicity of your project, but we might get some ideas from them.

I got the distutils command down to a one-liner, might be useful for the linux build

Cool, thanks. Works for python 3 as well.

tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 2, 2017
+ Revert PR9; it was never actually needed in the first place it just
  fixed an issue by side effect

+ add --no-deps to avoid building wheels for dependencies and to fix pypa#11
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 2, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 3, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
+ Revert PR9; it was never actually needed in the first place it just
  fixed an issue by side effect

+ add --no-deps to avoid building wheels for dependencies and to fix pypa#11
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
@YannickJadoul
Copy link
Member Author

I apparently automatically closed this because I (force-)updated my master branch to the cibuildwheel master. Let me see what I can do to reopen such that this conversation/issue does not get lost?

YannickJadoul added a commit to YannickJadoul/cibuildwheel that referenced this pull request Jul 9, 2017
@YannickJadoul YannickJadoul reopened this Jul 9, 2017
tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
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
Copy link
Contributor

tgarc commented Jul 9, 2017

I've attempted to address this in PR #18

tgarc added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 9, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 19, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 20, 2017
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 added a commit to tgarc/cibuildwheel that referenced this pull request Jul 20, 2017
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 added a commit that referenced this pull request Jul 20, 2017
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 #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).
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

3 participants