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

env: refactor IsolatedEnv #361

Closed
wants to merge 42 commits into from
Closed

env: refactor IsolatedEnv #361

wants to merge 42 commits into from

Conversation

layday
Copy link
Member

@layday layday commented Sep 28, 2021

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.

src/build/env.py Outdated

def __init__(self) -> None:
self._path: Optional[str] = None
def __init__(self, isolated_env_class: Optional[Type[IsolatedEnv]] = None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __init__(self, isolated_env_class: Optional[Type[IsolatedEnv]] = None) -> None:
def __init__(self, isolated_env_class: Type[IsolatedEnv] = _DefaultIsolatedEnv) -> None:

why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because _DefaultIsolatedEnv is defined underneath IsolatedEnvBuilder and I didn't wanna swap them around in the draft.

@uranusjr
Copy link
Member

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?

@layday
Copy link
Member Author

layday commented Sep 28, 2021

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)

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the API looks OK. @gaborbernat what do you think?

@layday don't forget to add an entry to the changelog about these breaking changes in the final version, it doesn't need to be anything too descriptive, just something on the lines of "reworked the IsolatedEnv interface and added an isolated_env_class argument to IsolatedEnvBuilder to make it easier to use custom environments".

@layday
Copy link
Member Author

layday commented Sep 28, 2021

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

@FFY00
Copy link
Member

FFY00 commented Oct 24, 2021

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

@layday
Copy link
Member Author

layday commented Oct 24, 2021

I'll try to work on it tomorrow.

@FFY00
Copy link
Member

FFY00 commented Oct 24, 2021

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

docs/conf.py Outdated Show resolved Hide resolved
@layday
Copy link
Member Author

layday commented Oct 25, 2021

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 layday force-pushed the adapt-env-for-pip branch 13 times, most recently from 307abd8 to 9f9ad5f Compare October 29, 2021 13:49
@layday layday marked this pull request as ready for review October 29, 2021 13:54
@layday
Copy link
Member Author

layday commented Jan 27, 2022

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

@pradyunsg
Copy link
Member

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

Wait, what?

@pradyunsg
Copy link
Member

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.

@layday
Copy link
Member Author

layday commented Mar 26, 2022

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 :)

@pradyunsg
Copy link
Member

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

@layday layday reopened this Mar 26, 2022
@FFY00
Copy link
Member

FFY00 commented Mar 26, 2022

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

henryiii added a commit to henryiii/build that referenced this pull request May 12, 2022
Co-authored-by: layday <layday@protonmail.com>
henryiii added a commit to henryiii/build that referenced this pull request May 12, 2022
Co-authored-by: layday <layday@protonmail.com>

Apply suggestions from code review

tests: add test for coverage
layday pushed a commit that referenced this pull request May 13, 2022
Co-authored-by: layday <layday@protonmail.com>

Apply suggestions from code review

tests: add test for coverage
@pradyunsg
Copy link
Member

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

@layday
Copy link
Member Author

layday commented Jul 29, 2022

Review my assumptions in #361 (comment) 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.

@henryiii
Copy link
Contributor

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.

@pradyunsg
Copy link
Member

  • 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.

@layday
Copy link
Member Author

layday commented Nov 10, 2022

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.

@FFY00
Copy link
Member

FFY00 commented Nov 10, 2022

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

@layday
Copy link
Member Author

layday commented Nov 21, 2022

Superseded by #537.

@layday layday closed this Nov 21, 2022
@layday layday deleted the adapt-env-for-pip branch March 1, 2024 18:41
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 this pull request may close these issues.

6 participants