build icon indicating copy to clipboard operation
build copied to clipboard

env: refactor `IsolatedEnv`

Open layday opened this issue 4 years ago • 27 comments

The IsolatedEnv is made responsible for env creation to allow pip to implement custom env creation logic.

~~IsolatedEnvBuilder takes an IsolatedEnv class as an argument so that bringing your own IsolatedEnv won't require re-implementing the builder.~~

layday avatar Sep 28 '21 09:09 layday

So from what I can understand, pip should implement its own IsolatedEnv subclass, pass it to the builder, and do something like

with builder as env:
    env.install_packages([...])
    subprocess.call([env.python_executable, "setup.py", "bdist_wheel"])  # Just to illustrate how to use the env.

right?

uranusjr avatar Sep 28 '21 15:09 uranusjr

Yep, that's right. Typical usage would be along the lines of:

with build.env.IsolatedEnvBuilder(PipIsolatedEnv) as env:
    builder = build.ProjectBuilder(
        source_dir,
        python_executable=env.python_executable,
        scripts_dir=env.scripts_dir,
    )
    env.install_packages(builder.build_system_requires)
    env.install_packages(builder.get_requires_for_build('wheel'))
    builder.build('wheel', output_dir)

layday avatar Sep 28 '21 15:09 layday

Yep, I just wanted to garner some feedback before diving any deeper.

layday avatar Sep 28 '21 15:09 layday

I could also use this in the bootstrap script that I am using.

FFY00 avatar Oct 24 '21 00:10 FFY00

I'll try to work on it tomorrow.

layday avatar Oct 24 '21 00:10 layday

No worries, I am currently monkeypatching build.env._create_isolated_env_venv, which is fine because I pull pypa/build as a submodule.

FFY00 avatar Oct 24 '21 00:10 FFY00

The new API implementation is complete and I'd welcome any feedback. I realise that this PR packs quite a bit and might be difficult to wade through. I could look at splittiing it up into smaller chunks but I wanna make sure everyone's okay with the direction this has taken first.

layday avatar Oct 25 '21 08:10 layday

The CI is also failing.

If anybody can figure out why the docs check is failing I'd be very interested to know...

layday avatar Oct 29 '21 15:10 layday

Right, it's because os.PathLike is not generic on Python 3.8. I'll bump the docs Python to 3.9 in tox.

layday avatar Oct 29 '21 15:10 layday

(I probably won't have time to work on this from next week so I'd appreciate any reviews over the weekend.)

layday avatar Oct 29 '21 18:10 layday

@FFY00 Would you like to review before merging?

layday avatar Oct 31 '21 10:10 layday

Sorry, I'll try to review this today!

For the lineno checks, let's get the function lineno and add the relative lines inside the function. Eg.

>>> import build.env
>>> build.env.IsolatedEnvBuilder.log.__code__.co_firstlineno + 10
138

FFY00 avatar Nov 12 '21 16:11 FFY00

Done, thanks for the suggestion.

layday avatar Nov 12 '21 16:11 layday

Thank you for the changes! I provided some more feedback. This is starting to look better, but still has some things I think should be changed :sweat_smile:

FFY00 avatar Nov 15 '21 23:11 FFY00

@FFY00 I'm approving this, anything else left to do?

gaborbernat avatar Dec 03 '21 17:12 gaborbernat

I'll try to review the new changes this weekend.

FFY00 avatar Dec 03 '21 20:12 FFY00

Just wanted to let you know that I haven't forgotten about this, I just have a bunch of things going on right now, and the holidays and end of year are making me even busier. I don't think there is any particular reason to rush this, as it is not critical to the affected parties, and they likely are on the same boat as me :sweat_smile:. I will try to get back to this in January.

FFY00 avatar Dec 23 '21 03:12 FFY00

To recap what has changed here in a major way:

  • The env builder is no more. IsolatedEnv has been stripped down to the bare essentials - currently that's just the two properties, environ and python_executable. This will allow pip to slot in its own isolated env but it does not make our own IsolatedEnv amenable to reuse by pip. This is because our requirements are fundamentally different. pip will not want pip to be installed by ensurepip or virtualenv; they will want to expose the running pip to the isolated env. pip maintainers have also not expressed interested in supporting virtualenv in place of or in addition to the stdlib venv like we do. Furthermore, pip supports a large number of installation options which our single-minded environment does not and that we would not be able to abstract as part of the isolated env protocol.
  • The runner has been modified to allow removing env vars from the environ - not just replacing them - as a precursor to #406. This has rendered the callbacks from pep517 incompatible with the project builder.

layday avatar Jan 07 '22 09:01 layday

Judging by https://github.com/pypa/pip/pull/10720, pip isn't interested in using build, so I'm closing this.

layday avatar Jan 27 '22 10:01 layday

Judging by pypa/pip#10720, pip isn't interested in using build, so I'm closing this.

Wait, what?

pradyunsg avatar Mar 26 '22 10:03 pradyunsg

As I see it, moving to a venv-based build environment in pip is the first step in making it possible for pip to move to using build, so I'm a genuinely confused by the closing note on this issue.

pradyunsg avatar Mar 26 '22 10:03 pradyunsg

I was given the impression that pip would be using venv directly in unison with pep517 (the library). The pip PR would have closed the pip issue where this PR was conceived and I had not heard from pip maintainers that they were interested in pursuing this further. If there is interest, all the code is still here :)

layday avatar Mar 26 '22 10:03 layday

In that case, do you want to reopen this? :)

pradyunsg avatar Mar 26 '22 13:03 pradyunsg

This will be one of my priorities for the PyCon sprints.

FFY00 avatar Mar 26 '22 17:03 FFY00

What can I do to help move this forward? :)

pradyunsg avatar Jul 29 '22 12:07 pradyunsg

Review my assumptions in https://github.com/pypa/build/pull/361#issuecomment-1007244397 and provide feedback on the isolated env interface - if there are any obvious methods or properties missing, if something doesn't feel right, etc. We are gonna need to rebase but the existing isolated env interface hasn't changed fundamentally since this PR.

layday avatar Jul 29 '22 12:07 layday

Personally stuck on the rebase. I started it, it's a pretty hefty one - the comments were not really very organized / squashed, so it was a lot of manual work. I think I was doing it in steps, or doing a manual merge. I then lost my spot and haven't restarted working on it. I've been traveling for a month and am pretty busy for the next week (workshop), but can probably work on this after that.

henryiii avatar Jul 29 '22 13:07 henryiii

  • This is because our requirements are fundamentally different.

That is correct, and not a problem. pip will likely create an environment with venv.EnvBuilder and use its internal runner script to run itself within that environment to install packages.

  • The runner has been modified to allow removing env vars from the environ

That seems fine? I guess I don't understand the implications of this fully though I imagine that pip's not gonna handle anything about this differently than it does today.

pradyunsg avatar Nov 10 '22 16:11 pradyunsg

That seems fine? I guess I don't understand the implications of this fully though I imagine that pip's not gonna handle anything about this differently than it does today.

We wanted to be able to clear a bunch of PYTHON* env vars like PYTHONHOME and PYTHONPATH. You can't do that with the stock pep517 subprocess wrappers. I don't remember the specifics anymore, if I'm honest. But it's a breaking change to the public build API.

layday avatar Nov 10 '22 18:11 layday

Yep, we need to create a subprocess with a controlled environment, where we remove some of these keys.

FFY00 avatar Nov 10 '22 18:11 FFY00