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

feat: support pypa build #521

Merged
merged 6 commits into from
Jun 23, 2021
Merged

feat: support pypa build #521

merged 6 commits into from
Jun 23, 2021

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jan 6, 2021

This will close #442.

@henryiii henryiii marked this pull request as draft January 6, 2021 15:25
@henryiii
Copy link
Contributor Author

henryiii commented Jan 6, 2021

Might be easier if pypa/manylinux#941 is accepted.

@henryiii
Copy link
Contributor Author

henryiii commented Jan 6, 2021

pypa-build builds in a virtual environment, so the path check in setup.py test is incorrect. It sees the executable we require in the first step, and the build virtual environment path in the second. Any ideas about how to change the check? Wouldn't an exact version check be enough, perhaps? We could provide a second version of it without this check? Etc.

@henryiii
Copy link
Contributor Author

henryiii commented Jan 6, 2021

This should probably be seen as a proof of concept until pypa/manylinux#941 goes in, because we currently don't touch the contents of the manylinux image.

@joerick
Copy link
Contributor

joerick commented Jan 15, 2021

BTW, I wouldn't be opposed to releasing PyPA build support before pypa/manylinux#941, given that this would be advertised as 'experimental' for now anyway.

@henryiii
Copy link
Contributor Author

I've included pypa/build in manylinux now, including manylinux1 for Python 2.7 (!), but we can't use it because we've pinned manylinux2010 to a version before that addition.

Honestly, I don't think dropping 2.7 support for manylinux2010 but keeping it for manylinux1 for now would be that big of an issue for a 1.11 release. We have very few users using manylinux2010 anyway, and even fewer of them making 2.7 wheels. Requires-Python support already helps, and we could check to see what Pythons are in the image and limit to that, producing an error if any are missed that are included by Requires-Python, and a warning if Requires-Python is not set.

@henryiii henryiii force-pushed the feat/pypa-build branch 2 times, most recently from 64c6668 to 60e7cdf Compare May 7, 2021 20:19
@henryiii
Copy link
Contributor Author

henryiii commented May 7, 2021

Does the PyPy image not contain build?

@mayeut
Copy link
Member

mayeut commented May 8, 2021

It seems they are base on an old manylinux2010 image so no pypa/build yet.
pypy/manylinux#16 should resolve this once merged.

@mayeut
Copy link
Member

mayeut commented May 8, 2021

pypy/manylinux#16 has been merged. The new image should have build (& auditwheel 4)

@henryiii henryiii force-pushed the feat/pypa-build branch 2 times, most recently from 3a77186 to 7b9b8e2 Compare May 17, 2021 21:45
@henryiii
Copy link
Contributor Author

Actually, it looks like pypy/manylinux is based on an old version of pypa/manylinux; notably it has a copy of requirements.txt internally (which is missing other important things we've added in this repo, including build).

Originally posted by @henryiii in pypa/manylinux#1055 (comment)

This means no build in pypy/manylinux, which is why this PR is failing.

@mayeut
Copy link
Member

mayeut commented May 24, 2021

Switched to quay.io/pypa/manylinux2010_x86_64 for PyPy while waiting for #671 just to see what the tests look like on GHA (need to rebase to get CI green)

@henryiii
Copy link
Contributor Author

Windows is failing on PyPy 7.3.3 + build:

Unable to copy 'C:\\cibw\\pypy3.7-v7.3.3-win32\\venvlauncher.exe'

@mayeut mayeut force-pushed the feat/pypa-build branch from d4d592e to 99dbec5 Compare May 25, 2021 16:48
@mayeut
Copy link
Member

mayeut commented May 25, 2021

Same error with PyPy 7.3.5

@henryiii henryiii mentioned this pull request May 25, 2021
12 tasks
@henryiii
Copy link
Contributor Author

pypa/build is tested on Windows + PyPy{2,3}, so not sure where this error is coming from.

I also think there are a few tests I need to update the output checking on, will look into that later.

@mayeut mayeut force-pushed the feat/pypa-build branch from 99dbec5 to 094d7c9 Compare May 27, 2021 12:13
@mayeut
Copy link
Member

mayeut commented May 27, 2021

@henryiii, rebased on main to get CI again. I fixed the cibuildwheel.linux.troubleshoot function to get all builds passing locally on linux. Let's see if CI agrees, there will only be the Windows PyPy issue left to investigate I think.

@henryiii
Copy link
Contributor Author

Looks like it's checking the exact pip versions, which is not true in the env created by build.

@mayeut
Copy link
Member

mayeut commented May 30, 2021

Looks like it's checking the exact pip versions, which is not true in the env created by build.

This is the issue for macOS.
For Windows PyPy, something else is going on.

@mayeut
Copy link
Member

mayeut commented May 31, 2021

I investigated a bit the issue with PyPy on Windows.
It seems the venv module does not like to be called from the python.exe symlink at all: https://foss.heptapod.net/pypy/pypy/-/issues/3479
Worked around for now by installing build[virtualenv]

@mayeut
Copy link
Member

mayeut commented May 31, 2021

The only test failing now is test/test_dependency_versions.py::test_dependency_constraints_file on macOS & Windows i.e.

Looks like it's checking the exact pip versions, which is not true in the env created by build.

Should we create/discuss something in pypa/build or should we just ignore this one ? see also pypa/build#292
My 2 cents, it's good to be able to pin things down for reproducibility. We might also want to extend that to linux builds (although it's a different issue) ?

@mayeut
Copy link
Member

mayeut commented May 31, 2021

I implemented the hack mentioned in pypa/build#292 just to see if everything passes with it. Not sure it's a good idea to implement such a hack.

@mayeut mayeut force-pushed the feat/pypa-build branch 2 times, most recently from 6f90957 to 592a270 Compare May 31, 2021 17:25
Comment on lines +283 to +310
# we use shell=True here for windows, even though we don't need a shell due to a bug
# https://bugs.python.org/issue8557
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 would be simpler with #672?

@henryiii
Copy link
Contributor Author

henryiii commented Jun 3, 2021

What's the status of this, @mayeut?

@mayeut
Copy link
Member

mayeut commented Jun 3, 2021

Short status:

  • all tests are passing whether build is the default or not (2e52bf2 really restored pip as the default)
  • We need to install build[virtualenv] to at least workaround PyPy venv issue, eventually to allow to pin pip when using build.

Open questions / things to do:

I'll most likely be offline for the next 2 weeks so not planning on adding more things here.

@joerick
Copy link
Contributor

joerick commented Jun 3, 2021

A thought on options design - rather than CIBW_PYPA_BUILD, could we change this to CIBW_BUILD_FRONTEND? (I don't love the name 'frontend' because what does that make us...) but that's the terminology used in PEP 517. Options would be pip or build.

More of a future-proofing measure, really. I've noticed some other tools offering themselves as build frontends... e.g. poetry might start building platform wheels, or something else like https://github.com/FFY00/trampolim might crop up... I suppose that in theory they should 'just' be invoking the build backend, but we can't predict the future!

@henryiii
Copy link
Contributor Author

I'll change CIBW_PYPA_BUILD to CIBW_BUILD_FRONTEND/tool.cibuildwheel.build-frontend after #684.

fix: update `cibuildwheel.linux.troubleshoot` to know about build

Workaround issue with PyPy venv module by installing build[virtualenv]

fix: test_dependency_constraints_file when using build

fix: test/test_pep518.py

fix: use `strtobool` to parse `CIBW_PYPA_BUILD`

fix: update `cibuildwheel.linux.troubleshoot` to know about `python -m pip wheel`

test: use `build_mode` in `test/test_dependency_versions.py` tests
@henryiii
Copy link
Contributor Author

I squashed the previous commits to make it easier to rebase and modify for the new config setup. I then switched to using build-backend. Docs still need to be added.

@henryiii henryiii marked this pull request as ready for review June 22, 2021 03:10
@henryiii
Copy link
Contributor Author

Put some basic docs in, happy to take improvements.

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.

Nice one @henryiii and @mayeut ! Just a few tweaks and this is ready to go, I think.

@joerick
Copy link
Contributor

joerick commented Jun 22, 2021

oops, i missed these earlier...

Yes, this does seem like a bit of a hack.. the use of PIP_CONSTRAINTS seems like exploiting an implementation detail of build. That said, it's unlikely to be a problem on the user's end, because we pin the version of build. I suppose the other option would be to use --no-isolation and ensure the correct versions of the tools we care about are installed in the environment before invoking build. I'd be okay with either, I think.

  • Extend tests ? all or select some that seem more relevant ? (already added those showing issues that needed a commit)

Since use of build is only an option, currently, this seems okay for now. I'd be curious if the test suite does pass with CIBW_BUILD_FRONTEND=build, though, did you try with the default as that?

@henryiii
Copy link
Contributor Author

I suppose the other option would be to use --no-isolation

I think this would break PEP 518; I think the current constraints is preferable for now.

I'd be curious if the test suite does pass with CIBW_BUILD_FRONTEND=build, though, did you try with the default as that?

I think that's how I started, and that's why I had that assert commented out; I was trying to run everything with build. But I could be wrong.

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.

[idea] pypa-build support
3 participants