Conversation
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: |
There was a problem hiding this comment.
| def __init__(self, isolated_env_class: Optional[Type[IsolatedEnv]] = None) -> None: | |
| def __init__(self, isolated_env_class: Type[IsolatedEnv] = _DefaultIsolatedEnv) -> None: |
why not?
There was a problem hiding this comment.
Because _DefaultIsolatedEnv is defined underneath IsolatedEnvBuilder and I didn't wanna swap them around in the draft.
|
So from what I can understand, pip should implement its own 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? |
|
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) |
There was a problem hiding this comment.
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".
|
Yep, I just wanted to garner some feedback before diving any deeper. |
|
I could also use this in the bootstrap script that I am using. |
|
I'll try to work on it tomorrow. |
|
No worries, I am currently monkeypatching |
5911a46 to
340af3b
Compare
c7f7ebb to
3583eed
Compare
|
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. |
307abd8 to
9f9ad5f
Compare
eee246e to
631a8d4
Compare
|
Judging by pypa/pip#10720, pip isn't interested in using build, so I'm closing this. |
Wait, what? |
|
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. |
|
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 :) |
|
In that case, do you want to reopen this? :) |
|
This will be one of my priorities for the PyCon sprints. |
Co-authored-by: layday <layday@protonmail.com>
Co-authored-by: layday <layday@protonmail.com> Apply suggestions from code review tests: add test for coverage
Co-authored-by: layday <layday@protonmail.com> Apply suggestions from code review tests: add test for coverage
|
What can I do to help move this forward? :) |
|
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. |
|
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. |
That is correct, and not a problem. pip will likely create an environment with
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 |
|
Yep, we need to create a subprocess with a controlled environment, where we remove some of these keys. |
|
Superseded by #537. |
The
IsolatedEnvis made responsible for env creation to allow pipto implement custom env creation logic.
IsolatedEnvBuildertakes anIsolatedEnvclass as an argumentso that bringing your own
IsolatedEnvwon't require re-implementingthe builder.