Adding support for building manylinux2010 wheels - alternative

[YannickJadoul]

This is a bit of a (cheeky/daring?) alternative proposal for supporting manylinux2010 and following versions (manylinux2014 has also been approved, recently).

Advantages:

• The new options CIBW_MANYLINUX_X86_64_IMAGE and CIBW_MANYLINUX_I686_IMAGE provide a nice generalization and should be future-proof. Rather than adding new environment variables every time a new manylinux standard is released, these new options encompass both the need to switch between different manylinux version as well as use custom images.
• We also don't need to keep on adding new Python-platform combinations to the python_configurations list.
• "Building for manylinux" and "choosing the version of manylinux/using a custom image" are made independent, now.

Disadvantages:

• Two breaking changes: CIBW_MANYLINUX1_X86_64_IMAGE and CIBW_MANYLINUX1_I686_IMAGE are not used anymore, and the *-manylinux1-x86_64 and *-manylinux1-i686 selectors in CIBW_BUILD and CIBW_SKIP will not work anymore. I think we can check for these, and warn the user (or even just fail) when an existing configuration still uses these, though.

The other option I see would to be just add a bunch of entries to python_configurations, associate a manylinux version with each of them, and create a CIBW_MANYLINUX_VERSION. I feel this would create a lot of repetition, though (CIBW_MANYLINUX*-X86_64/I686 environment variables, and cp??-manylinux*_x86_64/i686 build tags.

That being said, I'm not convinced this proposal is the perfect solution either, so I'd be grateful for some feedback (@joerick, @jbarlow83, @mayeut ?)

[YannickJadoul]

Cross-referencing #135

[YannickJadoul]

Grmbl. Of course tests are failing.

Reason: the name of the created wheels depends on the code, rather than on the cibuildwheel configuration... :-|

[jbarlow83]

manylinux2014 supports a lot of new platforms, so avoiding the Python configurations is the way to go. I don't think the breaking changes are a big deal - @joerick's instructions to pin the version of cibuildwheel used in configuration will help here.

Taking a step back a bit, I think concern about producing manylinux2010 wheels when we could produce manylinux1 is a bit overblown. manylinux1 is technically "manylinux2007"; we're worried about 12 year old vs 9 year old systems, both already rare in production. What would be rarer still is a system from that era in production that can install manylinuxX wheels. As far as I can tell pip couldn't install wheels until 2013, and I don't think it could install manylinux1 wheels until 2015. I suspect it would be very difficult to get a 2007 or 2010 distribution to the point where it could install manylinux wheels, and it may be well easier to build necessary packages from source. For example, Ubuntu 14.04 can't install wheels without PPAs. The biggest practical concern by far is that older versions of pip (<18) that can't install manylinux2010.

[joerick]

Okay. Fantastic! This is much better in my eyes! Here we have the default behaviour of building using the 2010 image (which will sometimes produce manylinux1 as a nice side effect for backward-compat), but users still have the freedom to build using manylinux1 image (a more advanced use-case imo) if they like. And we didn't need to add any more options to do it 👍

Three things are occurring to me-

• do we need to add an option to assert manylinux1 was produced when building on 2010? (Actually, maybe an explicit section of the docs that talks about manylinux1, and shows an assert done in the bash shell after the cibuildwheel run would be better)
• the CIBW_BUILD/CIBW_SKIP identifiers are really diverging from pip's now. We should include a list of them in the docs.
• breaking backward compat on the option. Yes, this should be okay, as @jbarlow83 says, I've always tried to encourage version pinning in cibuildwheel (always a good idea for repeatable builds), so it shouldn't be too big a problem. Maybe there's something clever we can do to rewrite the option? if 'manylinux1' in cibw_skip: cibw_skip.replace('manylinux1', 'manylinux'); print_warning()?

But thank you @YannickJadoul for continuing to push this forward!

[YannickJadoul]

@jbarlow83 Thanks for that! I hadn't actually realized the chronology of the manylinux1 being so much older than the PEP and its implementation in pip. So I agree this is a good enough default!

The biggest practical concern by far is that older versions of pip (<18) that can't install manylinux2010.

Indeed. And the main problem is probably that older pip versions cannot even detect the problem and warn the user to upgrade pip to install the wheels. Luckily, we could hopefully expect the average Linux user to be recently up-to-date or at least tech-savvy enough when maintaining an old system?

[YannickJadoul]

@joerick Great, good to hear you like this approach. Let's continue on this track then and hope I haven't overlooked any future problems :-)

do we need to add an option to assert manylinux1 was produced when building on 2010?

I don't know about this one. Is this going to be a common scenario, that users will want manylinux1 wheels even though they're building on manylinux2010? @jbarlow83 just argued that manylinux2010 is probably widely-compatible enough, by now. Then again, adding something to the docs is easy enough, and can only help to clarify further.

the CIBW_BUILD/CIBW_SKIP identifiers are really diverging from pip's now. We should include a list of them in the docs.

Yes, this also worried me for a second, but then I realized this was already the case before for some other identifiers (I can't remember which ones, though) But, for example, the ABI tags cpXXm are already not present either).

Maybe there's something clever we can do to rewrite the option? if 'manylinux1' in cibw_skip: cibw_skip.replace('manylinux1', 'manylinux'); print_warning()?

I like this idea! This will probably fix it, most of the time :-)
On a second thought, it might also be safer to just fail and have the user fix the options. As you said: if a user didn't want updates, the version should have been pinned.

One more thing: what do we do with the multiple wheels coming from cibuildwheel? I agree with the argument that every manylinux1 wheel is also a manylinux2010 wheel, but how confusing is that for the user? It makes our tests quite annoying, especially because we now don't have a 32-bit version, so we still don't get the same wheels as when building on manylinux1 images before.

I'll try to get this a bit closer to finished tonight. Now that we almost know how we want things, it shouldn't be too hard to finish!

[joerick]

One more thing: what do we do with the multiple wheels coming from cibuildwheel?

Offhand opinion- I think we just hand them both back to the user. They can upload both if they like. manylinux2010 wheels give the impression of a modern, secure build toolchain. I'd prefer to install a 2010 over a 1 given the choice!

[YannickJadoul]

They can upload both if they like. manylinux2010 wheels give the impression of a modern, secure build toolchain. I'd prefer to install a 2010 over a 1 given the choice!

Even though they are both exactly the same, except for name and metadata? :-D But true, that's also why I'm in doubt on what the best thing to do is...

[jbarlow83]

I'd think it's probably better for the packagers to provide all the wheels they can and let pip worry about what to install.

[YannickJadoul]

Finally, progress! Tests are succeeding, for the first time :-)

Sorry for all the force-push spam. Can I actually test this locally without polluting my own system too much, @joerick?

[joerick]

Great! I should get a chance to review in the next day or two

Sorry for all the force-push spam. Can I actually test this locally without polluting my own system too much, @joerick?

Yes, if you have Docker running locally you can run individual tests with `bin/dev_run_test test/00_test_name`

[YannickJadoul]

Great! I should get a chance to review in the next day or two

I/we still need to:

• Handle old CIBW_MANYLINUX1_..._IMAGE options and manylinux1 tags
• Add tests
• Add documentation

But I'm a bit stuck on (cleanly) getting the first one done.
I'll add the test from #135 later or tomorrow, and I'll make a list of things notes?

Yes, if you have Docker running locally you can run individual tests with bin/dev_run_test test/00_test_name

Ah yes, now that it's Linux-related, I can just do that, ofc. The problem was before on macOS or Windows PRs :-D

[YannickJadoul]

OK, ready for a first review, @joerick :-) And maybe @jbarlow83, @mayeut, if you two are interested? I'd love some more feedback on this.

A few concerns/things I'm not sure how to handle:

• I'm not a bash expert, so there's probably a better way to handle the multiple wheels in the bash array. I just put a for loop around chown {uid}:{gid} "/output/$(basename "$delocated_wheel")", for now, but any suggestions are welcome. Maybe this should become Python as well, at some point?
• The AUDITWHEEL_PLAT solution is less clean than I envisioned, mainly because we only have a single CIBW_ENVIRONMENT variable, but there are two different --platform options for auditwheel (auditwheel*_x86_64 and auditwheel*_i686), so you can't just say CIBW_ENVIRONMENT="AUDITWHEEL_PLAT=manylinux1". See test/06_docker_images.
  • The dockcross images do not have AUDITWHEEL_PLAT set by default, that's why we need to set them manually. They also contain auditwheel 2.0, and AUDITWHEEL_PLAT is only supported in 2.1, so that's why the update is there.
• Should we maybe turn these '1' and '2010' strings for default options into 'manylinux1' and 'manylinux2010'. More writing but also more clarity? I couldn't make up my mind.
• In __main__.py, I couldn't make up my mind how to organize the warnings/replacement of deprectated manylinux1 tags and options. There's the nice detect_warnings function, but by then it's too late to still adapt the new build_options (since it's already printed in the preamble, for example)? Maybe you have a great idea on the organization, there, @joerick?

[joerick]

Good stuff @YannickJadoul. I've left comments inline for most of your points above.

I don't totally understand the AUDITWHEEL_PLAT note, because I only see that referenced in the dockcross test - seems fine to me, if it's a quirk of this old image?

In main.py, I couldn't make up my mind how to organize the warnings/replacement of deprectated manylinux1 tags and options.

I suppose the detect_warnings approach isn't right since we don't even want to read the old obsolete options anymore. So maybe a detect_obsolete_options function that accesses os.environ, prints errors and exits the program? That could come before options parsing.

[joerick]

This manylinux_versions param... why is the default x86_64 only? I'd say the default would be to expect all wheels.

I'm wondering if perhaps a nicer signature would be like-
def expected_wheels(package_name, package_version, i686=True, x86_64=True, manylinux1=True)

Then tests can deselect the parts they don't need. (Just a suggestion!)

[YannickJadoul]

Interesting, myeah. I wasn't fully happy with the signature either, but:

why is the default x86_64 only?

Because currently there is only a manylinux2010_i686 standard, but not an image :-( See this reply. So, since the general consensus seems to be to move away from manylinux1, I made manylinux2010_x86_64 the default image for 64-bit, and nothing the default image for 32-bit.
Happy to change opinion, though, if you disagree. Or maybe we should make a PR with manylinux2010_i686 to make our code cleaner? :-P

I'm wondering if perhaps a nicer signature would be like-
def expected_wheels(package_name, package_version, i686=True, x86_64=True, manylinux1=True)

Problem is that I was trying to be future-proof as well. A PEP on manylinux2014 has already been approved, I believe, so I was hoping to limit the places where we'd need to copy-paste-replace2010by2014. But your signature is nicer, so I'd like to find a way in between!

[YannickJadoul]

Great! I'll start implementing and completing the changes!

I don't totally understand the AUDITWHEEL_PLAT note, because I only see that referenced in the dockcross test - seems fine to me, if it's a quirk of this old image?

Yeah, I guess so. I just thought: if someone uses a different image than the standard manylinux images, we can't detect what to pass to auditwheel as --plat option, but it's easy to use CIBW_ENVIRONMENT="AUDITWHEEL_PLAT=manylinux2010" (for example). However, that will not work, because AUDITWHEEL_PLAT also needs the _x86_64 and _i686 suffixes, so a simple CIBW_ENVIRONMENT addition will never really work.

In main.py, I couldn't make up my mind how to organize the warnings/replacement of deprectated manylinux1 tags and options.

I suppose the detect_warnings approach isn't right since we don't even want to read the old obsolete options anymore. So maybe a detect_obsolete_options function that accesses os.environ, prints errors and exits the program? That could come before options parsing.

Oh, you don't want to try and check/use the old options (cfr. your if 'manylinux1' in cibw_skip: cibw_skip.replace('manylinux1', 'manylinux'); print_warning() idea) ?

[YannickJadoul]

(Sorry, @mayeut; I wanted to get rid of the red icon showing "Changes requested", but I have no clue how to do so, so maybe I've been spamming your inbox with all kinds of GitHub mails, now 😄 )

[YannickJadoul]

If these CI builds succeed (and they should), I think this PR could (finally) be finished :-)
Sorry it took so long! In the end, writing the docs went quicker than I thought it would go.

@joerick Do you want to first merge this, or first merge your new docs? Because one of the two PRs will need to transfer the new/changed docs from the README to the new-style docs :-/

[mayeut]

Just a small change in README and this should be good to go

[joerick]

Just one teeny suggestion, but I think we're good to go.

[joerick]

Suggested change

	The format is `python_tag-platform_tag`. The tags are as defined in [PEP 0425](https://www.python.org/dev/peps/pep-0425/#details).
	The format is `python_tag-platform_tag`.

Our tweak from manylinux2010 to manylinux means we've deviated from this spec, so let's remove the link.

[YannickJadoul]

Ah, yes :-/ That's the downside of this PR, of course :-(

Maybe I'll still add something on using --print-build-identifiers to get the list.

[YannickJadoul]

I tried to rephrase and keep the reference (since it still seems relevant for users of cibuildwheel to know about this PEP).

Just a suggestions. If you don't agree and want to completely get rid of the sentence, do just push that change before you merge! :)

[joerick]

AAAAAAAaaaaand it's merged! Congrats @YannickJadoul !

[YannickJadoul]

WOOHOO! This deserves something of a party! :)

Once this gets released, we can let inform pypa/manylinux#179 as well :)

[YannickJadoul]

Thanks @joerick, @mayeut, @jbarlow83, and everyone else who gave input, as well!