Code refactor suggestions

[Czaki]

During contribution to cibuildwheel I found that some functions are to long and have many lines inside which are not connected with main task of this function.

My suggestions are:

• Pack build arguments in some namedtuple (class based with typing) or namespace. manylinux_images can be keep as separated arguement

• extract everything connected with setup python to install_python. It should:

  • install python
  • install pip if missed (I do not know why get_pip.pyis always called)
  • install base packages (setuptools, wheel)

  something like:

def install_python(python_configuration, constrains):
    ...

• extract some variables from main function to module level. For example repair_command_default can be changed to

repair_command_default_dict = {
    "linux": 'auditwheel repair -w {dest_dir} {wheel}',
    "macos": 'delocate-listdeps {wheel} && delocate-wheel --require-archs x86_64 -w {dest_dir} {wheel}'
}

and usage:

repair_command = get_option_from_environment('CIBW_REPAIR_WHEEL_COMMAND', platform=platform, default=repair_command_default_dict.get(platform, ""))

• share common commands between scripts (optional). To be sure that build commands (like pip wheel ... are same on every system maybe it is a good idea to put it in separat module and import?

[joerick]

Thanks @Czaki, these are good suggestions.

I think I agree with all the points above, except the last one because I like the readability of having the commands right there in the script.

[Czaki]

I think I agree with all the points above, except the last one because I like the readability of having the commands right there in the script.

From one side I agree with you (and this is why I put optional). Form other side I still think how to reduce chance of inconsistency between. systems. Maybe add something in unit test which will mock call/shellcals.

When #256 will be finished I can prepare PR with my proposition of this changes.

[joerick]

Maybe add something in unit test which will mock call/shellcals.

Sounds good. Yes I'd rather have a unit test that enforces consistency.

[YannickJadoul]

I agree the build options need to be refactored, yes. I've actually been trying out the type annotations and mypy, as suggested in #258, and this dictionary is one of the things that just doesn't seem to work with typing (I'll make a PR soon, just to be able to judge how things look, and to be able to further decide on it).

Apart from that, I'm not entirely convinced the other suggestions are that urgent? After taking out the install_cpython and install_pypy, things are already a lot more clear, and most of this code in windows.py, macos.py, linux.py is single-purpose anyway (i.e., dealing with quirks from the separate platforms, and not soon to be reused by different parts of cibuildwheel).
If we want more consistency between platforms, should we not rather think of a single "driver" file/class/function/..., that calls several separate platform-specific steps?

[Czaki]

I agree the build options need to be refactored, yes. I've actually been trying out the type annotations and mypy, as suggested in #258, and this dictionary is one of the things that just doesn't seem to work with typing (I'll make a PR soon, just to be able to judge how things look, and to be able to further decide on it).

repair_command_default_dict: typing.Dict[str, str] = {
    "linux": 'auditwheel repair -w {dest_dir} {wheel}',
    "macos": 'delocate-listdeps {wheel} && delocate-wheel --require-archs x86_64 -w {dest_dir} {wheel}'
}

problem with install_cpython and install_pypy is that it do not do complete python instillation. Still few steps (like install pip0is done in body of build function.

[YannickJadoul]

I agree the build options need to be refactored, yes. I've actually been trying out the type annotations and mypy, as suggested in #258, and this dictionary is one of the things that just doesn't seem to work with typing (I'll make a PR soon, just to be able to judge how things look, and to be able to further decide on it).

repair_command_default_dict: typing.Dict[str, str] = {
    "linux": 'auditwheel repair -w {dest_dir} {wheel}',
    "macos": 'delocate-listdeps {wheel} && delocate-wheel --require-archs x86_64 -w {dest_dir} {wheel}'
}

Yeah, no, that I know, but the problem is that the types of the keys of build_options are different and not checked vs. the build signature(s).

problem with install_cpython and install_pypy is that it do not do complete python instillation. Still few steps (like install pip0is done in body of build function.

Yes, I know, but that's mostly because it's shared (and important to see, because a few lines down it's added it to env). But it's worth a try to see if it's cleaner, maybe.

[Czaki]

Yes, I know, but that's mostly because it's shared (and important to see, because a few lines down it's added it to env). But it's worth a try to see if it's cleaner, maybe.

So there could be function install_python which get PythonConfiguration instance as argument. This function first call install_cpython or install_pypy and then execute common steps for both types.

Yeah, no, that I know, but the problem is that the types of the keys of build_options are different and not checked vs. the build signature(s).

Build options should be abandoned. There should be some object instead of this.

[Czaki]

@joerick
I observe that macos and windows build scripts are really similar. You also suggest (in #99) to rewrite linux to same scheme.
It show that almost the same code could be used on all systems.

This open question if the idea to write one code for all systems.
Of course It cannot be done with monolithic function because of temporary system bugfixes but I think it is possble to be done with class based build and split cone on several functions.

This may reduce code repetition, but it is possible to reduce code readability. What do you think about this?

[joerick]

It might be possible to unify, but whether it would be an improvement is an open question!

The transition of the linux script to the mac/windows 'call' style would be the first step. From there, it might be clearer how to proceed.

FWIW I'd be more inclined towards a single function with platform-specific support libraries, over a class-based override system. Class-based overrides are super-DRY, but you end up having to understand the whole hierarchy, how the methods are called, and what the base class implementation does, and how to override (call super or not?)... it ends up being much harder to read and understand.

At that point you'd be at a single function, with platform-specific implementations of 'install_python', 'call/shell', 'copy_file'... which might be an improvement, but I can still foresee lots of e.g. if platform == 'windows' stuff in that solution, especially for the 'temporary system bugfixes' you mention above.

All told, I reckon that if the individual platform build scripts were consistently styled/written, that might be the best solution. But I'm happy if somebody wants to explore this!

[Czaki]

FWIW I'd be more inclined towards a single function with platform-specific support libraries, over a class-based override system. Class-based overrides are super-DRY, but you end up having to understand the whole hierarchy, how the methods are called, and what the base class implementation does, and how to override (call super or not?)... it ends up being much harder to read and understand.

So maybe pass to function object which implement platform specific functions. Something like:

class System:
    def call(...):
        pass

    def virtualenv_path(...):
        pass

    def install_python(...):
        pass

[YannickJadoul]

This is something I've been thinking about as well. But I also agree that it will decrease our flexibility in adapting stuff. I'd wait a bit until the current rush of PRs is cooled down a bit more (with e.g., #239) and maybe then we can start with some simpler stuff (arguments to build and bash script to Python for Linux would be a nice place to start, I think).

I've also been playing with types, as suggested in #258, btw, just to see what comes out. There's a few things that need to be fixed, but afterwards I can open a PR for discussion. (Still really not convinced it's worth it, but it might a) give some insight into similarities, and b) provide a way to refactor with some static checks.)

[YannickJadoul]

Also, let's keep in mind to not change too many things that are not broken or that will not give clear advantages, I'd say. I like the refactoring idea of this single function, as a design in itself, but I'm not sure the current situation is problematic or much worse; to me, the main strength of cibuildwheel is not the code/architecture but about all the platform-specific worries we solve and hopefully save other people from having?

[YannickJadoul]

Note, #256 (comment)
It might be weird that we determine the options for different architectures (such as the different manylinux images), and report them in the preamble, but not for different OSes. If possible, it might be nice to take into account if we go around refactoring.