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

Add CLI #66

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,10 @@ home-page = "https://github.com/pradyunsg/installer"
description-file = "README.md"
classifiers = ["License :: OSI Approved :: MIT License"]
requires-python = ">=3.7"
requires = [
"build >= 0.2.0", # not a hard runtime requirement -- we can softfail
"packaging", # not a hard runtime requirement -- we can softfail
Comment on lines +14 to +15
Copy link
Member

Choose a reason for hiding this comment

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

Move these into an cli extra -- that's the mechanism for declaring optional dependencies. I don’t think all users need these to be installed.

Copy link
Member

Choose a reason for hiding this comment

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

And... assuming that we're reducing scope, we don't need either dependency.

This comment was marked as off-topic.

This comment was marked as off-topic.

]

[tool.flit.scripts]
python-installer = "installer.__main__:entrypoint"
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

I would strongly prefer to only provide python -m installer or even just a scripts/install_wheel.py in the sdist.

Both of them:

  • are unambigous about the Python executable being used.
  • do not result in conflicting executables given that these are unqualified.
  • do not require generating a script wrapper during installation of this package.

Choose a reason for hiding this comment

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

Yeah, it would also be good to have cli flags for settings stuff like the installation --root similar to setuptools, for example our setuptools target build/installs have to set a bunch of stuff so that things are built against the right interpreter and installed in the right place:

# Target setuptools-based packages
PKG_PYTHON_SETUPTOOLS_ENV = \
	_PYTHON_SYSCONFIGDATA_NAME="$(PKG_PYTHON_SYSCONFIGDATA_NAME)" \
	PATH=$(BR_PATH) \
	$(TARGET_CONFIGURE_OPTS) \
	PYTHONPATH="$(if $(BR2_PACKAGE_PYTHON3),$(PYTHON3_PATH),$(PYTHON_PATH))" \
	PYTHONNOUSERSITE=1 \
	_python_sysroot=$(STAGING_DIR) \
	_python_prefix=/usr \
	_python_exec_prefix=/usr

PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS = \
	--prefix=/usr \
	--executable=/usr/bin/python \
	--single-version-externally-managed \
	--root=$(TARGET_DIR)

Copy link
Member

Choose a reason for hiding this comment

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

Please see #58.

210 changes: 210 additions & 0 deletions src/installer/__main__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
"""Installer CLI."""

import argparse
import compileall
import distutils.dist
import pathlib
import platform
import sys
import sysconfig
import warnings
from email.message import Message
from typing import TYPE_CHECKING, Collection, Dict, Iterable, Optional, Sequence, Tuple

import installer
import installer.destinations
import installer.sources
import installer.utils
from installer.records import RecordEntry
from installer.utils import Scheme

if TYPE_CHECKING:
from installer.scripts import LauncherKind


class InstallerCompatibilityError(Exception):
"""Error raised when the install target is not compatible with the environment."""


class _MainDestination(installer.destinations.SchemeDictionaryDestination):
Copy link
Member

Choose a reason for hiding this comment

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

If you really think pycompile is such an important feature, then I’d rather that this be implemented in the SchemeDictionaryDestination object directly. I’m not convinced that it is, but I’m open to having my mind changed on this.

Copy link
Member

@layday layday Dec 11, 2021

Choose a reason for hiding this comment

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

Shouldn't the bytecode files be added to the record? I was under the impression pip did this too to have a "complete" record upon installation. In addition, some package managers (i.e. Nix) do not allow modifications to installed files and directories so if we don't compile them here then they'll either have to do it themselves or start-up performance will suffer rather substantially. The complication is that bytecode files are Python versioned so ideally these should be generated by the target interpreter.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh thanks, I missed that. I wonder why pip runs compileall then?

destdir: Optional[pathlib.Path]

def __init__(
self,
scheme_dict: Dict[str, str],
interpreter: str,
script_kind: "LauncherKind",
hash_algorithm: str = "sha256",
optimization_levels: Collection[int] = (0, 1),
destdir: Optional[str] = None,
) -> None:
if destdir:
self.destdir = pathlib.Path(destdir).absolute()
self.destdir.mkdir(exist_ok=True, parents=True)
scheme_dict = {
name: self._destdir_path(value) for name, value in scheme_dict.items()
}
else:
self.destdir = None
super().__init__(scheme_dict, interpreter, script_kind, hash_algorithm)
self.optimization_levels = optimization_levels

def _destdir_path(self, file: str) -> str:
assert self.destdir
file_path = pathlib.Path(file)
rel_path = file_path.relative_to(file_path.anchor)
return str(self.destdir.joinpath(*rel_path.parts))

def _compile_record(self, scheme: Scheme, record: RecordEntry) -> None:
if scheme not in ("purelib", "platlib"):
return
for level in self.optimization_levels:
target_path = pathlib.Path(self.scheme_dict[scheme], record.path)
if sys.version_info < (3, 9):
compileall.compile_file(target_path, optimize=level)
else:
compileall.compile_file(
target_path,
optimize=level,
stripdir=str(self.destdir),
)

def finalize_installation(
self,
scheme: Scheme,
record_file_path: str,
records: Iterable[Tuple[Scheme, RecordEntry]],
) -> None:
record_list = list(records)
super().finalize_installation(scheme, record_file_path, record_list)
for scheme, record in record_list:
self._compile_record(scheme, record)


def main_parser() -> argparse.ArgumentParser:
"""Construct the main parser."""
parser = argparse.ArgumentParser()
parser.add_argument("wheel", type=str, help="wheel file to install")
parser.add_argument(
"--destdir",
"-d",
metavar="/",
type=str,
default="/",
help="destination directory (prefix to prepend to each file)",
)
parser.add_argument(
"--optimize",
"-o",
nargs="*",
metavar="level",
type=int,
default=(0, 1),
help="generate bytecode for the specified optimization level(s) (default=0, 1)",
)
parser.add_argument(
"--skip-dependency-check",
"-s",
action="store_true",
help="don't check if the wheel dependencies are met",
)
return parser


def get_scheme_dict(distribution_name: str) -> Dict[str, str]:
"""Calculate the scheme disctionary for the current Python environment."""
scheme_dict = sysconfig.get_paths()

# calculate 'headers' path, sysconfig does not have an equivalent
# see https://bugs.python.org/issue44445
dist_dict = {
"name": distribution_name,
}
distribution = distutils.dist.Distribution(dist_dict)
Copy link
Member

@layday layday Oct 23, 2021

Choose a reason for hiding this comment

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

I don't know if it matters but this approach is incompatible with pip. pip checks if the installation's performed in a virtualenv and constructs a seemingly pip-specific include path:

$ python -V
Python 3.9.6
$ python -c 'import pip._internal.locations
  print(pip._internal.locations.get_scheme("foo").headers)'
[...]/venv/include/site/python3.9/foo
$ python -c 'import distutils.dist
  command = distutils.dist.Distribution({"name": "foo"}).get_command_obj("install")
  command.finalize_options()
  print(command.install_headers)'
[...]/venv/include/python3.9/foo

Copy link
Member

Choose a reason for hiding this comment

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

Also, to note, installer will have to depend on setuptools for distutils if this is gonna work in Python 3.12 and beyond.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why pip is doing that, this is all a huge mess.

See the bpo linked above, we want to fix this before 3.12 so that there is a migration path.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, is there anybody who actually expects to find includes at either of those locations? Maybe the best thing to do is not to install them at all and emit some kind of warning:

There is no default location for includes in a virtual environment; skipping

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the main case I know of where a Python package installs header files that other packages want to use as includes is numpy. Perhaps because of these issues, numpy actually ships the header files inside its package directory, and packages that want to use them call numpy.get_include() to find them (e.g. here in h5py).

I'm new enough to the world of compiled packages that I don't know the history of how it came to be this way, or whether there are other models in use.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the root issue is that python-config simply does not work on virtual environments. This will hopefully get fixed soon.

Copy link
Member

Choose a reason for hiding this comment

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

This sort of stuff is honestly why I am wary of providing a CLI that automatically determines the right paths for you. :)

install_cmd = distribution.get_command_obj("install")
assert install_cmd
install_cmd.finalize_options()
# install_cmd.install_headers is not type hinted
scheme_dict["headers"] = install_cmd.install_headers # type: ignore

return scheme_dict


def check_python_version(metadata: Message) -> None:
"""Check if the project support the current interpreter."""
try:
import packaging.specifiers
except ImportError:
warnings.warn(
"'packaging' module not available, "
"skipping python version compatibility check"
)
return

requirement = metadata["Requires-Python"]
if not requirement:
return

versions = requirement.split(",")
Copy link
Member

Choose a reason for hiding this comment

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

Use SpecifierSet?

for version in versions:
specifier = packaging.specifiers.Specifier(version)
if platform.python_version() not in specifier:

Choose a reason for hiding this comment

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

Maybe use sysconfig.get_python_version() instead since that should generally work better for cross compilation scenarios I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, but unfortunately sysconfig.get_python_version does not contain the minor version.

Choose a reason for hiding this comment

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

Docs indicate it does:

Return the MAJOR.MINOR Python version number as a string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry! I meant the patch version.

raise InstallerCompatibilityError(
"Incompatible python version, needed: {}".format(version)
)


def check_dependencies(metadata: Message) -> None:
"""Check if the project dependencies are met."""
try:
import build
import packaging # noqa: F401
except ModuleNotFoundError as e:
warnings.warn(f"'{e.name}' module not available, skipping dependency check")
return

missing = {
unmet
for requirement in metadata.get_all("Requires-Dist") or []
for unmet_list in build.check_dependency(requirement)
for unmet in unmet_list
Copy link
Member

@layday layday Oct 23, 2021

Choose a reason for hiding this comment

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

This will flatten the dependency chain. For example, if you depend on build, and build depends on packaging, and packaging depends on pyparsing and pyparsing is not installed, the output will be:

Missing requirements: build, packaging, pyparsing

Or in a random order, since this is a set:

Missing requirements: pyparsing, build, packaging

Which is probably a tad confusing.

}
if missing:
missing_list = ", ".join(missing)
raise InstallerCompatibilityError(
"Missing requirements: {}".format(missing_list)
)


def main(cli_args: Sequence[str], program: Optional[str] = None) -> None:
"""Process arguments and perform the install."""
parser = main_parser()
if program:
parser.prog = program
args = parser.parse_args(cli_args)

with installer.sources.WheelFile.open(args.wheel) as source:
# compability checks
metadata_contents = source.read_dist_info("METADATA")
metadata = installer.utils.parse_metadata_file(metadata_contents)
check_python_version(metadata)
Copy link
Member

Choose a reason for hiding this comment

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

Should there be an option to skip this check as well? Or use the existing skip-dependency-check option to skip this as well?

I'm pretty sure there are times when I've bumped Requires-Python to reflect what I test and care about, even though there's a good chance code will still run on an older Python version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have strong opinions one way or the other about a python version check, but FWIW when I originally read this code I assumed the dependency check had a skip option because it was assumed people might often be installing a wheel when its dependencies are not currently installed, but will be by the time it is expected to be used.

In contrast, skipping the python version check is saying that you think the wheel metadata is flat out wrong. Which I guess you're saying it might sometimes be...

Copy link
Member

Choose a reason for hiding this comment

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

Yup. If you want a concrete example, see h5py/h5py#1727 - CI is slow, so we had an incentive to stop testing on Python 3.6. When we did, we could either declare that it requires Python 3.7 (which would be wrong at least for a while), or declare that it still supports 3.6 - until someone one day realises that that's wrong and files an issue.

Being overly restrictive is generally easier for maintainers - someone who does a simply pip install h5py on Python 3.6 will automatically get an older version that was tested on 3.6. And if someone really needs a newer h5py, they can bypass the check and see if it works.

Choose a reason for hiding this comment

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

In contrast, skipping the python version check is saying that you think the wheel metadata is flat out wrong. Which I guess you're saying it might sometimes be...

The main thing here I think is that version checks need to be done against the target interpreter sysconfig(the one that will be used with the package being installed) and not the one actually executing installer and doing the installation(which can be entirely different for example when cross compiling)

if not args.skip_dependency_check:
check_dependencies(metadata)

destination = _MainDestination(
get_scheme_dict(source.distribution),
sys.executable,
installer.utils.get_launcher_kind(),
optimization_levels=args.optimize,
destdir=args.destdir,
)
installer.install(source, destination, {})


def entrypoint() -> None:
"""CLI entrypoint."""
main(sys.argv[1:])


if __name__ == "__main__":
main(sys.argv[1:], "python -m installer")