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

Factor out run_on_each #560

Open
henryiii opened this issue Jan 25, 2021 · 1 comment
Open

Factor out run_on_each #560

henryiii opened this issue Jan 25, 2021 · 1 comment

Comments

@henryiii
Copy link
Contributor

henryiii commented Jan 25, 2021

I'd like to make the following change. The current structure of the build function looks like this:

def build(options: BuildOptions) -> None:
    temp_dir = Path(tempfile.mkdtemp(prefix='cibuildwheel'))
    built_wheel_dir = temp_dir / 'built_wheel'
    repaired_wheel_dir = temp_dir / 'repaired_wheel'

    try:
        if options.before_all:
            log.step('Running before_all...')
            env = options.environment.as_dictionary(prev_environment=os.environ)
            before_all_prepared = prepare_command(options.before_all, project='.', package=options.package_dir)
            call([before_all_prepared], shell=True, env=env)

        python_configurations = get_python_configurations(options.build_selector)

        for config in python_configurations:
            log.build_start(config.identifier)
            ...

I'd like to refactor it like this:

def build(options: BuildOptions) -> None:
    try:
        if options.before_all:
            log.step('Running before_all...')
            env = options.environment.as_dictionary(prev_environment=os.environ)
            before_all_prepared = prepare_command(options.before_all, project='.', package=options.package_dir)
            call([before_all_prepared], shell=True, env=env)

        python_configurations = get_python_configurations(options.build_selector)

        for config in python_configurations:
            with TemporaryDirectory(prefix='cibuildwheel') as tmp_dir:
                build_on_each(config, env, Path(tmp_dir))

    except subprocess.CalledProcessError as error:
        log.error(f'Command {error.cmd} failed with code {error.returncode}. {error.stdout}')
        sys.exit(1)


def build_on_each(config, env: Dict[str, str], tmp_dir: Path) -> None:
    built_wheel_dir = temp_dir / 'built_wheel'
    repaired_wheel_dir = temp_dir / 'repaired_wheel'

    log.build_start(config.identifier)
    ...

This provides a new temporary directory that is always cleaned up for each build configuration, is much better for unit testing and mocking, and removes the ridiculous levels of indentation we have for 90% of the code in build and that is keeping us from applying good programming practices, like context managers (and auto formatting? ;) ). I also think it's much easier to read the try/except, and "build_on_each" is a clearly defined concept, and still reads nicely linearly. See #558. Should wait until at least #484 goes in.

@joerick
Copy link
Contributor

joerick commented Jan 25, 2021

Well, yes, it is kinda tempting. In fact, I already did it once in #458, but rolled it back when I decided not to use context managers for the logging.

In that example, eh, it wasn't as clean as I had hoped. I think the complication for the linux one is that many configs share one container, so each build_one invocation carries a lot of baggage.

I'm probably too close to judge, and certainly not willing to say that the level of indentation in linux.py especially is good per se, but there might be a readability benefit to keeping it as one function (or 'build script') versus breaking it out? not sure.

But if we do decide to change it, we should try to do so when we don't have a lot of big PRs around - those merges could be scary!

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 a pull request may close this issue.

2 participants