refactor: split generic workflow from OS specific workflow #996

mayeut · 2022-01-22T14:01:25Z

This PR started as an attempt to close #560 in the backlog.
As mentioned in the issue, there's probably little benefit in doing just this so I went further than that just to get some ideas where we could go. This is just a draft to see if any of those ideas are worth looking into, I don't expect this PR to be merged ever.

The end game idea was to split generic stuff from OS specific stuff which could help for #78, #676, #966, #971
To help reviewing/checking if anything is of interest:

Virtual Environments are now handled as context managers for either build or tests
There's now a PlatformBackend abstract class which should be implemented for all that's related to call/shell/file operations. This allows to have Windows/macOS to implement a "native/direct" one, linux the DockerContainer one.
There's now a BuilderBackend class which handles the default build workflow thus avoiding code duplication for generic stuff. It can be customized to handle OS specific workarounds, specific implementations.

As a whole:
Pros:

Better workflow consistency between all OS (and we could enforce the same constraints on linux as on windows/macOS easily for even more consistency) with less code duplication.
Allows easier extendability through abstract class (one might even suggest a plug-in system as an end goal)

Cons:

Higher complexity
Harder to see the order of specific patch/workarounds/specifics at OS level

henryiii

I did a quick first pass. In general, I like this direction as long as it's easy to unit test (mocking and such should be easy?). I'll need to do a second pass looking not at the diff, but at the full code, to see if it is still reasonable to track what is happening as a reader. Also to see why there's a backend and a platform backend.

I'm not sure if @joerick will like it, given https://cibuildwheel.readthedocs.io/en/stable/contributing/#design-goals, but since it's this far, I think a "final version" should be compared with the current version (probably not through a diff) to see if it looks better, to see if it looks easy to make large specific changes without breaking the other platforms, etc. (PS: to be clear, I'm not claiming to like it yet either, just positive about the direction but will need further review later.)

henryiii · 2022-01-25T17:31:14Z

cibuildwheel/backend.py

+from .virtualenv import VirtualEnv, VirtualEnvBase
+
+
+class BuilderBackend:


I think this would be a lot simpler as a dataclass.

I will need some time to think about what it would look like going this way.

henryiii · 2022-01-25T17:32:05Z

cibuildwheel/backend.py

+        return wheels
+
+
+def test_one(


Starting with test_ makes this look like a pytest test.

changed to do_test_wheel

henryiii · 2022-01-25T17:34:09Z

cibuildwheel/platform_backend.py

+from cibuildwheel.util import call, ensure_virtualenv, shell
+
+
+class PlatformBackend:


Shouldn't this be an ABC? (either through the ABCMeta metaclass or inheriting from abc.ABC)?

henryiii · 2022-01-25T17:42:51Z

PS: Also, regardless of the final comparison, there are some good changes here we should pull out, like more Final usage.

henryiii · 2022-01-25T17:51:32Z

By the way, I've thought about this too, and what I likely would have attempted would have been to implement smaller methods and a Protocol that would verify that a backend had the required components. Protocols are generally nicer than ABC's, though it's easier to share code with an ABC (which is not always great for reading the code, which is part of why Protocols are nice...).

joerick · 2022-01-27T21:59:04Z

just a quick note - i'm very curious to review this but I won't have time for a few days! Apologies

joerick · 2022-02-04T19:09:46Z

Thanks for this @mayeut, it's interesting! Reflections on a quick 30-min skim through...

Reading the code itself (outside of the diff), I get a bit lost. For example, it's not very clear to me where the actual building takes place. The existing codebase, you can go to the build function of the platform you're interested in and read it from top-to-bottom to understand what happens. I like that - especially in the context of building things, a 'build script' style of code is more suitable I think. In this PR, we lose this straight-forwardness. Of course, the existing codebase pays for that in some code duplication, but I think readability is important enough to warrant this.
I'm surprised how 'big' BuilderBackend is. It's got a lot of functionality in there, so perhaps suffers from the age-old problems of inheritance - where execution flow gets pretty confusing. Perhaps there would be a different version of this which tried to make the platform-specific bits as a Protocol, with as small a surface-area as possible. This might be nicer. But I'm not sure how small that surface area could get...
Maybe it's possible to retain the build-script style, but with state-less (or minimally stated) utilities for the platform-specific bits via a Protocol? I've thought about this a little... possibly yes, but I guess we'd have to have loads of if platform == 'windows': and if platform == 'macos': for the various workarounds. I'm not sure if this would be more or less readable... potentially it could be more of a mess than the existing code.
I know that Lines of Code is not a great benchmark for a number of reasons, but I am surprised that this increases the count, given the duplication that was eliminated. I suppose we pay for it in genericness.

mayeut · 2022-02-05T10:31:16Z

@joerick, @henryiii, thanks for taking the time to review this.

I'll try to address the comments but not sure when I'll be able to address all of them.
In the meantime, if there are bits that you think should be extracted from this on their own, please let me know (as @henryiii did for Final usage).

mayeut · 2022-02-05T11:29:27Z

I rebased on main. Only the last 3 commits are addressing some of the concerns raised during review.

mayeut · 2022-02-05T11:47:11Z

The existing codebase, you can go to the build function of the platform you're interested in and read it from top-to-bottom to understand what happens. I like that - especially in the context of building things, a 'build script' style of code is more suitable I think. In this PR, we lose this straight-forwardness. Of course, the existing codebase pays for that in some code duplication, but I think readability is important enough to warrant this.

@joerick, I probably went too far in BuilderBackend: as I was doing _setup_build_env, I thought it might be good for other steps to be functions as well. I removed functions for those other steps and expanded them in BuilderBackend::run. It still does not answer your concern but is probably better now than before in this regard.

joerick · 2022-04-29T20:32:31Z

There are some good ideas here, I think it was worth attempting for sure, but ultimately I don't think that the extra layers of abstraction give us enough upside in maintainability to outweigh the downsides of indirectness.

I'm cleaning up PRs, so I'll close this one for now. Ofc feel free to reopen if you'd like to continue discussion.

henryiii reviewed Jan 25, 2022

View reviewed changes

mayeut mentioned this pull request Feb 5, 2022

chore: use Final type annotation on every top-level constant #1010

Merged

mayeut added 23 commits February 5, 2022 11:47

refactor: extract build_one

89d098d

refactor: new_tmp_dir context manager

3d50dda

refactor: extract install_python from setup_python

822c1e2

refactor: extract build_one_build & build_one_test from build_one

aa6e80c

refactor: move common build code to its own file

9e52096

refactor: move common build code to its own file

3e20d52

refactor: move common test code to its own file

3e73414

refactor: use a class for common build code

29fd28a

refactor: use a class for virtualenv

2749f5c

refactor: prepare linux

ff24c3b

refactor: prepare linux 2

00c72c4

refactor: prepare linux 3

1c30f94

refactor: prepare linux 4

39c347f

refactor: test_one is now common to all platforms

9f447c1

refactor: venv sanity check

0d86814

refactor: venv sanity check 2

e2ba402

refactor: again

73bfd9a

remove dead code

4190519

refactor: before_all

9cc6560

Update backend.py

aeac0a3

Update test_before_all.py

b20b844

refactor: context managers

0054461

review: rename test_one → do_test_wheel

1df5d5a

mayeut added 2 commits February 5, 2022 12:04

review: more linear read for BuilderBackend::run

4926908

review: PlatformBackend should inherit from abc.ABC

bf171ab

mayeut force-pushed the issue-560 branch from df6c1bc to bf171ab Compare February 5, 2022 11:27

joerick closed this Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split generic workflow from OS specific workflow #996

refactor: split generic workflow from OS specific workflow #996

mayeut commented Jan 22, 2022

henryiii left a comment •

edited

Loading

henryiii Jan 25, 2022

mayeut Feb 5, 2022

henryiii Jan 25, 2022

mayeut Feb 5, 2022

henryiii Jan 25, 2022

mayeut Feb 5, 2022

henryiii commented Jan 25, 2022

henryiii commented Jan 25, 2022

joerick commented Jan 27, 2022 •

edited

Loading

joerick commented Feb 4, 2022

mayeut commented Feb 5, 2022

mayeut commented Feb 5, 2022

mayeut commented Feb 5, 2022

joerick commented Apr 29, 2022

		from .virtualenv import VirtualEnv, VirtualEnvBase


		class BuilderBackend:

		from cibuildwheel.util import call, ensure_virtualenv, shell


		class PlatformBackend:

refactor: split generic workflow from OS specific workflow #996

refactor: split generic workflow from OS specific workflow #996

Conversation

mayeut commented Jan 22, 2022

henryiii left a comment • edited Loading

Choose a reason for hiding this comment

henryiii Jan 25, 2022

Choose a reason for hiding this comment

mayeut Feb 5, 2022

Choose a reason for hiding this comment

henryiii Jan 25, 2022

Choose a reason for hiding this comment

mayeut Feb 5, 2022

Choose a reason for hiding this comment

henryiii Jan 25, 2022

Choose a reason for hiding this comment

mayeut Feb 5, 2022

Choose a reason for hiding this comment

henryiii commented Jan 25, 2022

henryiii commented Jan 25, 2022

joerick commented Jan 27, 2022 • edited Loading

joerick commented Feb 4, 2022

mayeut commented Feb 5, 2022

mayeut commented Feb 5, 2022

mayeut commented Feb 5, 2022

joerick commented Apr 29, 2022

henryiii left a comment •

edited

Loading

joerick commented Jan 27, 2022 •

edited

Loading