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

feat: use external pip if available #736

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Feb 19, 2024

Trying #733 out. Pip is used if it is in the same environment as build and new enough. Saves a bit of time setting up the venvs. Haven't worked on tests yet.

From quick testing building both an sdist and wheel for itself with virtualenv installed this goes from about 4.7 seconds to 3.2 seconds. With venv only, it goes from 10.5 seconds to 4.4 seconds.

FYI, with uv (hacked in) it took 2.36 secs.

(This is a very simple build, of course, with only a small handful of deps)

uv's experimental native build support took 571ms, but that's only building the wheel (above is SDist and wheel).

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii marked this pull request as ready for review February 23, 2024 00:29
src/build/env.py Outdated

try:
return _has_valid_pip()
except ModuleNotFoundError: # Python 3.7 only
Copy link
Member

Choose a reason for hiding this comment

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

Why does this happen?

Copy link
Member

@layday layday Feb 25, 2024

Choose a reason for hiding this comment

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

I would prefer if we always threw this from _has_valid_pip instead of the cryptic StopIteration, which arguably you should never have to handle, especially at a distance. It would also match importlib's implementation of distribution (singular):

try:
    return next(cls.discover(name=name))
except StopIteration:
    raise PackageNotFoundError(name)

(PackageNotFoundError being a subclass of ModuleNotFoundError.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. And the error was if importlib_metadata was missing, though we have it as a dep, so that would only be for bootstrapping. This makes it have the same signature as pip missing, though, so no need to special case.

src/build/env.py Outdated
@@ -83,13 +83,29 @@ def _has_valid_pip(purelib: str) -> bool:
else:
from importlib import metadata
Copy link
Member

Choose a reason for hiding this comment

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

Import this from _importlib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about that. I always call it _compat/importlib (and have a _compat/typing, etc).

Copy link
Member

Choose a reason for hiding this comment

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

+1 to rename it to _compat/importlib.

@@ -70,7 +70,7 @@ def _minimum_pip_version() -> str:
return '19.1.0'


def _has_valid_pip(purelib: str) -> bool:
def _has_valid_pip(**distargs: object) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we didn't lose the typing here, but Python doesn't make it easy:

if typing.TYPE_CHECKING:
    from typing_extensions import NotRequired, TypedDict, Unpack

    class _Distargs(TypedDict):
        path: NotRequired[list[str]]

...

def _has_valid_pip(**distargs: Unpack[_Distargs]) -> bool:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it along for now, would prefer a _compat/typing module if we add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that in a separate PR. I also usually have _compat/tomllib (https://github.com/scikit-build/scikit-build-core/tree/main/src/scikit_build_core/_compat for example).

src/build/env.py Outdated
@@ -139,6 +155,12 @@ def python_executable(self) -> str:
"""The python executable of the isolated build environment."""
return self._python_executable

def _pip_args(self, *, isolate: bool = False) -> list[str]:
Copy link
Member

@layday layday Feb 25, 2024

Choose a reason for hiding this comment

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

This doesn't appear to ever be called without isolate being true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, dropped the arg.

src/build/env.py Outdated
@@ -201,7 +221,11 @@ def _create_isolated_env_virtualenv(path: str) -> tuple[str, str]:
"""
import virtualenv

cmd = [str(path), '--no-setuptools', '--no-wheel', '--activators', '']
if _valid_global_pip():
cmd = [str(path), '--no-seed', '--activators', '']
Copy link
Member

Choose a reason for hiding this comment

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

The path is already a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just inherited from previous code, but okay, changed.

src/build/env.py Outdated
_subprocess([executable, '-m', 'pip', 'install', f'pip>={_minimum_pip_version()}'])

# Avoid the setuptools from ensurepip to break the isolation
_subprocess([executable, '-m', 'pip', 'uninstall', 'setuptools', '-y'])
if _valid_global_pip():
Copy link
Member

Choose a reason for hiding this comment

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

Redundant, setuptools is not installed if with_pip is false. (It is also not installed on Python 3.12+.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay, yes. Removed this branch.

@henryiii henryiii force-pushed the henryiii/feat/externalpip branch 2 times, most recently from c5ac342 to afe6f02 Compare February 26, 2024 17:22
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Copy link
Member

@layday layday left a comment

Choose a reason for hiding this comment

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

Thanks!

@layday layday merged commit 894998a into pypa:main Feb 26, 2024
61 checks passed
@henryiii henryiii deleted the henryiii/feat/externalpip branch February 26, 2024 20:41
@cr1901 cr1901 mentioned this pull request Feb 29, 2024
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.

2 participants