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

[Feature Request] Optionally build wheels with bdist_wheel instead of pip #486

Closed
Time0o opened this issue Dec 19, 2020 · 26 comments
Closed

Comments

@Time0o
Copy link

Time0o commented Dec 19, 2020

I'm in the following situation: I'm trying to build a a C++/Python project structured like this:

├── CMakeLists.txt
├── include
├── source
└── python
    ├── setup.py
    └── ...

i.e. Python code and bindings are separated from the main C++ source code.

Now, I'm unable to to build this project with cibuildwheels because when pip wheel ... is executed from the package directory (python in this case), pip copies the contents of that directory to a temporary directory before running setup.py. At this point there is no clean way (it seems) of referencing my project's root directory from setup.py anymore so the toplevel CMakeLists.txt can't be found and the build fails.

I have previously used multibuild which offers the option of building with python setup.py bdist_wheel instead, avoiding this problem. I think it would be sensible of making this possible with cibuildwhells as well by adding a command line option and modifying linux.py etc. Is this pull-request worthy?

EDIT: to clarify, my setup.py invokes CMake to build the C++ code as well as the Python bindings.

@Czaki
Copy link
Contributor

Czaki commented Dec 19, 2020

As I understand your cmake call does not depend on the python version? In such a case the cmake call should be done before all build using CIBW_BEFORE_ALL
Usage of {project} is described in test command documentation but apply also to CIBW_BEFORE_ALL

Also, Your project structure does not allow to create, a source bundle because sdist cannot collect C++ source code. The problem with bdist_wheel is that it does not provide build isolation between python versions.

So if your cmake depends on the python version then make may not found that there is a need to recompile part of thew code.

@YannickJadoul
Copy link
Member

As I understand your cmake call does not depend on the python version? In such a case the cmake call should be done before all build using CIBW_BEFORE_ALL

That doesn't seem like a great solution, in general.

I'm a bit surprised though that we would have never had this problem before. We have had #294/#295/#319. The original issue, #294, might be interesting to look at, though.

@joerick
Copy link
Contributor

joerick commented Dec 19, 2020

Perhaps this could be solved by disabling pip's build isolation?

CIBW_ENVIRONMENT: "PIP_NO_BUILD_ISOLATION=1"

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

@Czaki I hope I understand this correctly, I'm not well versed in Python package distribution:

My project is built either by just using the toplevel CMakeLists.txt, in which case just the C++ code is built or by running setup.py which builds the C++ code + Python bindings. The latter runs CMake as part of the build_ext step (and passes PYTHON_EXECUTABLE=... to CMake in the process so it does depend on the Python version).

I'm currently only interested in binary distributions so I haven't given sdist any thought. I'm not sure I understand the build isolation part, wouldn't every invocation of python setup.py bdist_wheel create a unique (per Python version) wheel?

@Czaki
Copy link
Contributor

Czaki commented Dec 19, 2020

My project is built either by just using the toplevel CMakeLists.txt, in which case just the C++ code is built or by running setup.py which builds the C++ code + Python bindings. The latter runs CMake as part of the build_ext step (and passes PYTHON_EXECUTABLE=... to CMake in the process so it does depend on the Python version).

So what in your code guarantee that python dependent parts will be recompiled if build without build isolation? Or you plan to create a separate cibuildwheel call per python version?

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

My project is built either by just using the toplevel CMakeLists.txt, in which case just the C++ code is built or by running setup.py which builds the C++ code + Python bindings. The latter runs CMake as part of the build_ext step (and passes PYTHON_EXECUTABLE=... to CMake in the process so it does depend on the Python version).

So what in your code guarantee that python dependent parts will be recompiled if build without build isolation? Or you plan to create a separate cibuildwheel call per python version?

I'm still not sure I understand so I'll explain what I did when I just built wheels on manylinux "manually":

Inside the Docker container, for every Python installation under /opt/python, I ran /opt/python/.../bin/python setup.py bdist_wheel -d wheelhouse inside my python directory. As far as I can tell that built everything from scratch every time (maybe not the most efficient way to do it but my project is not that large) and created separate, per Python version, files under python/build and python/wheelhouse. Isn't that what would happen with cibuildwheel as well?

@Czaki
Copy link
Contributor

Czaki commented Dec 19, 2020

And how did you clean the build result between python setup.py bdist_wheel -d wheelhouse calls?

cibuildwheel use build isolation to be sure that some artifacts from the previous build does not pollute the next wheel.

There are two options. Cleaning is built inside your setup script (or you use temporary dir for build in cmake) then

CIBW_ENVIRONMENT: "PIP_NO_BUILD_ISOLATION=1"

should be enough
or you need not add proper clean command inside

CIBW_BEFORE_BUILD: clean_command

@henryiii
Copy link
Contributor

henryiii commented Dec 19, 2020

So what you really want is to be able to control the “.” in pip wheel .? Or the running directory of pip (though the . is the better thing to change I think so your output doesn’t get nested).

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

I think my main problem is that setuptools isn't really meant to work with files not included in the build directory and its subdirectories. So maybe I have to rethink what I'm doing. Alternatively it's possible to set TMPDIR=. to make pip place all build files in the cwd, similar to running bdist_wheel.

@YannickJadoul
Copy link
Member

YannickJadoul commented Dec 19, 2020

So what you really want is to be able to control the “.” in pip wheel .? Or the running directory of pip (though the . is the better thing to change I think so your output doesn’t get nested).

This is possible, though:

positional arguments:
  package_dir           Path to the package that you want wheels for. Must be
                        a subdirectory of the working directory. When set, the
                        working directory is still considered the 'project'
                        and is copied into the Docker container on Linux.
                        Default: the working directory.

https://cibuildwheel.readthedocs.io/en/stable/options/#command-line-options

(cfr. #294/#295/#319, mentioned above)

@henryiii
Copy link
Contributor

A few points now that I'm at a keyboard:

  • You should never build with python setup.py bdist_wheel. That is considered an implementation detail by the core devs, and may not do the right thing if you do not import setuptools first (pip injects setuptools's modifications directly before invoking bdist_wheel). The pip command should be flexible enough to do any build you need. python -m build should also, I think, but that's still young and we don't have support for it here yet.
    • I don't think this is the real issue, here, though.
  • If you run cibuildwheel in the /python directory, cibuildwheel will mount the subfolder only in the docker run, so "PIP_NO_BUILD_ISOLATION=1"will not fix it (though, since you are using artifacts/code from outside the build directory, you will need that setting to use pip from inside the/python` directory)
  • The ideal solution I think is to run pip wheel python instead of pip wheel ., but I don't think we have a hook for that. I'd verify it works locally first!
  • Pip at least is designed to work with projects that are in a folder, though it's not common - most packages put setup.py at the top level so that pip install git+https://git.repo/some_repo.git works. But you can do pip install git+https://git.repo/some_repo.git#subdirectory=python - I've had to work with a package structured like that before. So I think setuptools might support it (though possibly with PIP_NO_BUILD_ISOLATION=1 / --no-build-isolation passed to pip).

@henryiii
Copy link
Contributor

Ahh, @YannickJadoul points out we do have a hook for that. Yes, that sounds like exactly what you need.

@YannickJadoul
Copy link
Member

YannickJadoul commented Dec 19, 2020

Ahh, @YannickJadoul points out we do have a hook for that. Yes, that sounds like exactly what you need.

But that fixes the copying into Docker for manylinux, but not the issue with pip wheel only copying the directory with setup.py?

OK, maybe with what you said here:

  • Pip at least is designed to work with projects that are in a folder, though it's not common - most packages put setup.py at the top level so that pip install git+https://git.repo/some_repo.git works. But you can do pip install git+https://git.repo/some_repo.git#subdirectory=python - I've had to work with a package structured like that before.

@henryiii
Copy link
Contributor

PIP_NO_BUILD_ISOLATION fixes that issue, I believe.

@henryiii
Copy link
Contributor

Actually, maybe it doesn't: #466 (comment)

Sounds like you still need a step where you collect the outer directories. You have access to everything inside the sdist command so you could collect what you need there. I believe it actually copies the SDist and unpacks it into a new directory. It is designed to catch broken SDists - you are not supposed to distribute without an SDist.

Or, as a last ditch effort, you could use build-backend = "setuptools.build_meta:__legacy__", which does not do the copy.

@henryiii
Copy link
Contributor

@YannickJadoul
Copy link
Member

Ech. This amount of workarounds would almost suggest adding bdist_wheel is not so bad after all. But I'm not a huge fan either, and it's not future proof/good practice, ofc. I'm just surprised this kind of issue still hasn't been solved...

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

Actually, maybe it doesn't: #466 (comment)

Sounds like you still need a step where you collect the outer directories. You have access to everything inside the sdist command so you could collect what you need there. I believe it actually copies the SDist and unpacks it into a new directory. It is designed to catch broken SDists - you are not supposed to distribute without an SDist.

Or, as a last ditch effort, you could use build-backend = "setuptools.build_meta:__legacy__", which does not do the copy.

That seems like an idea. Thanks, I'll try it. It's unfortunate that this isn't really supported out of the box. Naturally it makes sense to me not have the setup.py in the project root directory when creating Python bindings to a C++ codebase. That way all the Python stuff could e.g. be moved into a separate repository and added as a submodule.

I none of this works I guess I'll have to ditch that idea though.

@henryiii
Copy link
Contributor

I think build-backend = "setuptools.build_meta:__legacy__" would fix it directly; the reason that modifying the sdist command is better would be you get something useful out of it - a working SDist.

We should not support broken packages out of the box. Packages are supposed to be build with pip or build, not by manually applying commands to setup.py. I would never recommend adding python setup.py bdist_wheel as encouraging bad practice - though if we allowed the entire command to be replaced (as has been discussed and rejected in the past), then this could be manually done by a desperate user.

I have several codebases like that - and the general rule is, put setup.py in the top level, and everything else in /python. That way, you are friendly to Python users, you only have 2 extra files in the main dir instead of one, and all the normal commands "just work". Though, admittedly, you'll likely want a pyproject.toml and a setup.cfg, so now that's grown to 4 files...

But a user who doens't read your docs can just write pip install "blah", where blah can even be a remote git repo, and it works. I should not have to learn about custom packaging to build your package, it should work using standard commands out of the box. That's the idea, anyway.

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

I think build-backend = "setuptools.build_meta:__legacy__" would fix it directly; the reason that modifying the sdist command is better would be you get something useful out of it - a working SDist.

We should not support broken packages out of the box. Packages are supposed to be build with pip or build, not by manually applying commands to setup.py. I would never recommend adding python setup.py bdist_wheel as encouraging bad practice - though if we allowed the entire command to be replaced (as has been discussed and rejected in the past), then this could be manually done by a desperate user.

I have several codebases like that - and the general rule is, put setup.py in the top level, and everything else in /python. That way, you are friendly to Python users, you only have 2 extra files in the main dir instead of one, and all the normal commands "just work". Though, admittedly, you'll likely want a pyproject.toml and a setup.cfg, so now that's grown to 4 files...

But a user who doens't read your docs can just write pip install "blah", where blah can even be a remote git repo, and it works. I should not have to learn about custom packaging to build your package, it should work using standard commands out of the box. That's the idea, anyway.

That's a good argument. I think I'll go back to a toplevel setup.py. At least this was a learning experience :D

@henryiii
Copy link
Contributor

henryiii commented Dec 19, 2020

I am curious if you drop a pyproject.toml in /python with:

[build-system]
requires = [
    "setuptools>=42",
    "wheel"
]
build-backend = "setuptools.build_meta:__legacy__"

And then build with cibuildwheel python if that would work. Though I think structuring would be better in the long run. (Again, it's perfectly to have all other Python code / bindings / CMake in the /Python folder).

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

I am curious if you drop a pyproject.toml in /python with:

[build-system]
requires = [
    "setuptools>=42",
    "wheel"
]
build-backend = "setuptools.build_meta:__legacy__"

And then build with cibuildwheel python if that would work. Though I think structuring would be better in the long run. (Again, it's perfectly to have all other Python code / bindings / CMake in the /Python folder).

I might try this too. For now I can't get the toplevel setup.py + everything else under python/ to work either though I'm not sure if this is another limitation or a problem on my end. The python/ directory looks like this:

python/
├── mpsym
│   ├── __init__.py
│   └── _mpsym_tests.py
└── source
    ├── CMakeLists.txt
    └── _mpsym.cpp

But I can't specify packages=['python/mpsym'] inside the toplevel setup.py because this will install everything to site-packages/python/mpsym.

@henryiii
Copy link
Contributor

henryiii commented Dec 19, 2020

This is exactly like having a src directory. Follow the instructions for that. See setuptools docs or https://scikit-hep.org/developer/packaging

Basically, set package_dir to python.

@Time0o
Copy link
Author

Time0o commented Dec 19, 2020

Ah, I got it. I think that clears up all my questions. That will hopefully allow me to use cibuildwheel without issues.Thanks a lot again!

@Time0o Time0o closed this as completed Dec 19, 2020
@YannickJadoul
Copy link
Member

Thanks for holding on and trying out, and being open to the alternative suggestions, @Time0o! :-)

@jbms
Copy link

jbms commented Jan 28, 2021

I can't really find any documentation regarding:

build-backend = "setuptools.build_meta:__legacy__"

but it does not appear to have any effect as far as pip copying to a temporary directory. In my own build, I tried adding it to my pyproject.toml and it had no effect.

Also, from looking at the source code, there isn't any indication that it could have such an effect:

https://github.com/pypa/setuptools/blob/b6bbe236ed0689f50b5148f1172510b975687e62/setuptools/build_meta.py#L225

It appears that the legacy backend has only minor differences compared to the normal backend.

The real fix would be: pypa/pip#9091

However, it isn't clear when/if that may be merged.

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

No branches or pull requests

6 participants