-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
@pradyunsg how do you want to implement the bytecode generation? I was thinking a custom destination, or making |
I went with returning the records from |
@pradyunsg I think I have a pretty complete draft, let me know if the implementation looks good to you so that I can start working on the tests. |
@pradyunsg friendly ping. |
Ah cool! great to see someone working on it! I hope we have this soon, it fills such a big gap! |
52c59d9
to
37107a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good start to me. I've left some comments inline.
Should there also be a --prefix
option or similar, to install a package to a different location from the default?
# compability checks | ||
metadata_contents = source.read_dist_info("METADATA") | ||
metadata = installer.utils.parse_metadata_file(metadata_contents) | ||
check_python_version(metadata) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
How do you know that would be the correct path for the interpreter you are targeting? |
It would be up to the user to provide a useful prefix. As I understand it, this is meant to be a low-level tool for expert users such as downstream packagers, so it doesn't need to be totally foolproof. I don't know if the option would be necessary, though. Of downstream packaging systems I've got some familiarity with:
|
IMO, overwriting the prefix is in most cases a very bad way to handle things. The only reason I can think of where one might want to overwrite the prefix is if you are targeting a different interpreter altogether, but doing that this way is very flimsy, prone to breaking, and limited in situations it might actually work (often you'll need to overwrite more than just the prefix). Even if you are an expert, I would not recommend this tool for that. For that scenario, I would recommend writing a new tool using the API this project provides that allows to properly customize the install locations -- either overwrite or allow to evaluate the schemes with all supported config options, not just the prefix. In most cases the best way to handle customizations in the install locations is by adding new schemes to sysconfig. This PR does not implement it yet, but I would like to have an option to select the scheme for which you want to install.
This should be done via a sysconfig prefix IMO.
Which demonstrates the limitations of overwriting the prefix, and would be require a different tool like the one I described above. But if spack is the one shipping the Python interpreter, I would recommend adding a sysconfig scheme instead 😅
Conda needs much more control over the environment, not just the locations supported by wheels/sysconfig, so it brings its own logic. |
Fair enough. Installing to a specified prefix is a familiar pattern on Unix-y platforms, though, both for Python packages and more generally. So I'd hope we could at least offer some kind of explanation about "if you're looking for a prefix option, here's what to do instead". 🙂
Conda packages of Python code are often made using |
I think there is a big difference between:
IMHO prefix should not be supported, unless the CLI also intends to somehow offer support for installing into a python interpreter that is not sys.executable. In this case, I'd expect, say, a |
Alternatively, to cover the /usr vs /usr/local case, you'd need a python interpreter patched to properly support this (in other words, it actually adds that directory to sys.path and allows importing the code you just installed), e.g. the debian package for python, which should probably be exposed in the CLI as a --scheme option to select which install scheme you want to utilize. This would also allow installing in --user mode by diverting from posix_prefix to posix_user. |
Fair enough. So the "what to do instead" would be to run |
If you’re interested, this is my current understanding:
I don’t think a simple path prefix (=destdir) is the right abstraction for |
That is not exactly accurate, |
fc78b4b
to
110c255
Compare
@pradyunsg can you have a look at this? If everything looks alright to you, I will start working on the tests. |
97c0ae1
to
b30ada1
Compare
for more information, see https://pre-commit.ci
This might be controversial, but I'd actually favour not installing a script, and having the CLI only available as This avoids some possible forms of confusion - e.g. if you activate a virtualenv where |
I concur, and happy to remove that from the PR.
I don't think removing the entrypoint changes much in relation to bootstrapping, as most people that need this tool still need to build the wheel from source. And if you have access to the source, you could already just run it from source to install itself instead of unpacking, which is much much more quirky. |
That's true. Though I'm guessing downstream packagers would implement the 'unzip a wheel' bit for bootstrapping |
FWIW, in https://github.com/FFY00/python-bootstrap I am just running everything from source. Well, kinda, setuptools needs some special handling 🙃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for filing this, and for your patience here! I've had a bunch of review comments written down for months, but life stuff kept preventing me from having time to sit down and writing this blurb.
There's a few aspects of this that I'm wary of, which can be summarized down to a difference between what various folks think this package is.
- IMO it is providing one very narrowly defined thing: logic to properly unpack wheels into the specified directories with all the right launchers.
- ISTM folks expect this to be a drop-in replacement for
pip install {options} {wheel}
with other sorts of useful functionality.
This has various implications relevant to this PR:
-
I don't want to get into the business of figuring out what the "right" directories are for unpacking a wheel in this package.
Various redistributors do things differently, and I don't want to be involved in handling those differences here, beyond having them specify the right paths to this package. To paint a fuller picture on this --
pip
now uses sysconfig on 3.10+ and distutils for <3.9. However, there's an opt-in environment variable to adopt sysconfig. There are multiple kinds of patches flying around, on pip and Python. There's also a private only-for-redistributors mechanisms to change whether pip uses sysconfig or disutils under consideration, that I certainly don't want to be replicating all of that work here. These are the sorts of details that redistributors need to understand and make decisions about on/at their end, and they are in the best position to specify the relevant paths for each Python installation they provide. -
I do not want tie the usage to the currently running version of Python.
While
pip
can only install packages into the running interpreter, this package does not have the same limitations in the way it is implemented. There's no reason that someone can't install a wheel into Python 2.7 or 3.3 or whatever, while running this library on Python 3.10. I don't think the CLI for this package should imply that either. -
I'm not sure I would want this package to handle checking extra things.
Functionality like checking dependencies or the
python_version
are inherently tied to the running interpreter. Other than the what-is-the-scope aspects of this, this is also at odds with the note above, about not being tied to the running interpreter. Some of this stuff sounds like the sort of thing that can (and should) be handled at a different level, i.e. a higher level tool. This package isn't intended to be used by "end users", i.e. a regular user of Python software -- things like dependency management and ensuring that the installs are happening properly are a concern for higher level tooling, that's built around or on top of this package.
Because this is fairly long already... there's broadly two directions here:
- Make a case that this project should be broader than "unpack this wheel correctly" and why that is a better, more sustainable and more maintainable position for the overall ecology here.
- Rework this PR to fit into the smaller scope of "unpack this wheel correctly" (or file a separate one).
"build >= 0.2.0", # not a hard runtime requirement -- we can softfail | ||
"packaging", # not a hard runtime requirement -- we can softfail |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
[tool.flit.scripts] | ||
python-installer = "installer.__main__:entrypoint" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see #58.
"""Error raised when the install target is not compatible with the environment.""" | ||
|
||
|
||
class _MainDestination(installer.destinations.SchemeDictionaryDestination): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
dist_dict = { | ||
"name": distribution_name, | ||
} | ||
distribution = distutils.dist.Distribution(dist_dict) |
There was a problem hiding this comment.
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. :)
I think it probably makes sense to go even further and have this allow for sdist/repo based build+install workflows rather than just wheel based workflows, now this could be done in
Maybe it would make sense to have
Yep, this is very important for cross compilation where we have a host toolchain that has a python interpreter built for the host but needs to install for a target interpreter(which can not be run on the host)
Well using sysconfig may work in some cases when targeting a different interpreter, for cross compilation we hack up the sysconfig for target package builds so that stuff like c extensions gets built against the target interpreter rather than the host interpreter. |
versions = requirement.split(",") | ||
for version in versions: | ||
specifier = packaging.specifiers.Specifier(version) | ||
if platform.python_version() not in specifier: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Yeah, i realize that, I mean it would potentially make sense to make it a package, or make a meta package with namespace vendored dependencies. |
This is getting a bit off-topic, so it would be better to open an issue in its repo, but I don't really see why it would be beneficial to make it a package. Note, by "package" here I mean a Python installable and distributable package, not a package as in Python module at the interpreter level (it already is that, as it is a folder with a |
@jameshilliard Pradyun wants to keep the scope of this package tightly focused, and it currently (in I'd say a CLI is useful even if it requires users to specify the same information as the Python API - all 6 destination directories ( It's tempting to say it should then extend to more traditional methods of describing install locations, i.e. I can imagine it might be useful to have a shortcut to install for the currently running Python, if this were 100% explicit, e.g. Maybe it's worth asking @hroncok. He mentioned on pypa/flit#483 that Fedora would like to move to using |
Totally agree. I was also thinking that “do current python by default and allow a way to specify others” might be the sweet spot. Future consideration: Hmm, there should be some kind of database that contains all those paths for known OSs and distros. Or a file format standard so people can build a database like this easily themselves and we could supply all options at once by pointing to a file like that.
I thought we already agreed that Agreed about other, more complex options, but this one is both trivial and useful. |
These paths can be dynamic as of 3.10, so this is not possible. |
I've toyed with some of the ideas that have been thrown around here at pyproject-install. Rewiring the installation prefix feels like a hack and |
I've had a go in #92 at an alternative CLI. It requires much more verbose input than this one, because it doesn't use any information from the Python which runs |
Yeah, this approach seems like a good starting point as we want to customize this anyways in general. |
FYI this can't really be done in a separate tool package without creating a dependency cycle as this hypothetical tool wouldn't be able to depend on |
Closing this out, since #94 is merged now! :) |
Closes #52
Signed-off-by: Filipe Laíns [email protected]