-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-75229: ensurepip does not honour the value of $(prefix) #17634
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
Conversation
Co-Authored-By: Xavier de Gaye <xdegaye@gmail.com>
@pganssle: Would you mind to have a look at this PR? Thanks in advance. |
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.
Taking a look on request by @pganssle.
The change looks good to me. I'm not sure why someone would want to have --prefix specified to ensurepip but if they want to be able to, this looks right to me.
I can't say that I know much about My one reservation is that this only adds a test using a mock, it doesn't actually test that this solves the problem - it assumes that |
I think this is the crux of whether or not to accept this PR to me. To be honest I'm not entirely sure what If this is just about cross-compiling Python, would it be enough to set |
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.
Hi @ZackerySpytz! Would you be willing to update this PR with the current main
, so that it is up to date?
(the second scenario here has helpful guidance)
Misc/NEWS.d/next/Build/2019-12-16-17-50-42.bpo-31046.XA-Qfr.rst
Outdated
Show resolved
Hide resolved
@pradyunsg: @ZackerySpytz is not very active these days, you should copy this PR to create a new one. |
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
Lib/ensurepip/__init__.py
Outdated
@@ -120,27 +120,27 @@ def _disable_pip_configuration_settings(): | |||
os.environ['PIP_CONFIG_FILE'] = os.devnull | |||
|
|||
|
|||
def bootstrap(*, root=None, upgrade=False, user=False, | |||
def bootstrap(*, root=None, prefix=None, upgrade=False, user=False, |
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.
Add prefix
at the end of the args list, as noted above. Also, supplying both root and prefix arguments doesn't really make sense (and may even be rejected by pip at some point in the future, if it isn't already) so it would be better to make these arguments mutually exclusive.
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.
Thinking about it, I'm not sure they should be mutual exclusive.
If root
equals $(DESTDIR)
and prefix
equals $(prefix)
, I would guess we want to install into $(DESTDIR)/$(prefix)
. I need to look more closely at this later today.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@@ -1981,7 +1981,7 @@ install: @FRAMEWORKINSTALLFIRST@ commoninstall bininstall maninstall @FRAMEWORKI | |||
install|*) ensurepip="" ;; \ | |||
esac; \ | |||
$(RUNSHARED) $(PYTHON_FOR_BUILD) -m ensurepip \ | |||
$$ensurepip --root=$(DESTDIR)/ ; \ | |||
$$ensurepip --prefix=$(prefix) ; \ |
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 know enough about the Unix build process to know if using --prefix
instead of --root
is the right thing to do here. I'm going to mark my review as resolved (and approve the PR) but I'd recommend getting someone who knows how Unix/MacOS installs work to check this part of the change before merging (unless, of course, you are such a person and you're happy it's right 🙂)
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 can test it on *nix and macOS, but I know very little about pip/ensurepip :)
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.
OK. Looking at the pip source, --prefix
corresponds roughly to the sysconfig
scheme posix_prefix
. And --root
can be used with --prefix
, it basically rebases the installation directories on the root
directory.
So if you have prefix=/foo/bar
and root=/bax/quux
, then you'd get purelib
pointing to /baz/quux/foo/bar/lib/pythonX.Y/site-packages
. With just prefix
you'd get /foo/bar/lib/pythonX.Y/site-packages
. And with just root
you'd get /baz/quux/lib/pythonX.Y/site-packages
.
There's all sorts of qualifications and special cases (because this is packaging 🙁) but that's the basic idea.
Given that the original issue was around cross-compilation for Android, I have zero idea how any of this would impact that actual scenario, though...
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 guess you'd use --root
for the directory where your target rootfs is being populated, and then you'd use --prefix
for the path within the target rootfs.
cc. @gpshead, since you dabble with embedded systems IIRC.
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'm not the right person to answer this either, i've poked #build on discord to see if anyone else is.
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 can confirm that using --prefix
for the expected location in the destination filesystem and --root
(=$DESTDIR
) for the current location of the virtual filesystem where the package is being prepared makes total sense. (and is how setup.py
works )
Having only one or the other leads to headaches;
TL;DR: We expect --prefix
to change the install location of files and the path they use to reference one another while --root
should change the location we put files without changing how they refer to one another
With a basic build process and environment as such :
SOURCE_DIR=/tmp/py3.12.3
TARGET_ROOT=/tmp/build.out
TARGET_PREFIX=/opt/python/py3.12.3
cd ${SOURCE_DIR}
configure --prefix=${TARGET_PREFIX} --with-ensurepip=no [...]
make
make DESTDIR=${TARGET_ROOT} install altinstall
We get these results :
${TARGET_ROOT}${TARGET_PREFIX}/bin/python3 -m ensurepip
head -n 1 ${TARGET_ROOT}${TARGET_PREFIX}/bin/pip
#!/tmp/build.out/opt/python/py3.12.3/bin/python3
${TARGET_ROOT}${TARGET_PREFIX}/bin/python -m ensurepip --root ${TARGET_ROOT}
head -n 1 ${TARGET_ROOT}${TARGET_ROOT}${TARGET_PREFIX}/bin/pip
#!/tmp/build.out/tmp/build.out/opt/python/py3.12.3/bin/python3
In both cases the script installed contains the ${TARGET_ROOT}
in the shebang when it shouldn't, but specificaly using --root
behaves the way I would expect --prefix
to work if no prefix had been set in the configure script.
The expected install locations for pip3 should be:
--prefix
:${PREFIX}/bin/pip3
with shebang#!${PREFIX}/bin/python3
--root
(assuming no prefix) :${ROOT}/bin/pip3
with shebang#!/bin/python3
--prefix
and--root
:${ROOT}${PREFIX}/bin/pip3
with shebang#!${PREFIX}/bin/python3
The behaviour becomes even weirder when using --with-ensurepip=
(install
|upgrade
) in configure
.
It triggers a call to something equivalent to :
LD_LIBRARY_PATH=${SOURCE_DIR} ${SOURCE_DIR}/python -E -m ensurepip [--upgrade] --root=${DESTDIR}
But this call ignores the --root
parameter completely and just acts on the host system if an installation using the same prefix exists. The -E parameter makes setting PYTHONHOME
irrelevant but omitting it doesn't resolve the issue.
With thing the way they are currently, with-ensurepip=no
is the only option making sense in configure. After that, I see two quick and dirty solutions :
- calling
${TARGET_ROOT}${TARGET_PREFIX}/bin/python3 -m ensurepip
on the build host, and fixing the shebang in the scripts afterwards - adding
${TARGET_PREFIX}/bin/python3 -m ensurepip
as a post-install action on the destination host
I agree that the proper solution would be to have the makefile call something along the lines of:
$$ensurepip --prefix=$(prefix) ; \ | |
$$ensurepip --root=$(DESTDIR)/ --prefix=$(prefix) ; \ |
I'm not sure the trailing /
is necessary.
One issue remains when cross-compiling (eg. AARCH64 binaries on a X86_64 host), the build host (x86_64) must be able to execute the target's binaries (AARCH64) for the actual call to work. Assuming ensurepip
is pure python, the call should use the host's python3
binary (x86_64) with --prefix
and --root
rather than the one being built (AARCH64).
I don't see it as a blocking issue since the person building the package should be able to tell if they can use --with-ensurepip
or if they should rather call ensurepip
from the build host's python rather than letting the Makefile guess/try.
EDIT:
I forgot to mention that the actual module installation path follows the behaviour shown with the scripts and shebang. I just used them to illustrate the expected and actual behaviours
EDIT2: Added TL;DR an some clarifications
@@ -122,6 +123,12 @@ Module API | |||
*verbosity* controls the level of output to :data:`sys.stdout` from the | |||
bootstrapping operation. | |||
|
|||
*prefix* specifies the directory prefix to use when installing. |
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 think we should explain the difference between root and prefix better. Per now, it is not obvious how they differ.
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 agree it's not obvious. And that's speaking as a pip maintainer... It's all shrouded in mystery and legacy behaviour dating back to the origins of distutils, I think. And it's very much to do with Unix - on Windows, those options are essentially never used (which is mostly why I don't understand them...)
But I think it's fair to assume that anyone using these options is doing so because they know of the same options in pip, and so they understand what they are doing (or at least their confusion isn't our problem). It's not ideal, but OTOH, this isn't the place for a tutorial on packaging concepts, either.
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.
root vs prefix goes back to the base unix model I think, and may be explained in make documentation or something similarly old.
Something along the lines: the prefix (for installation) can be /usr or /usr/local or /opt/my-program but the root directory (for building) is /tmp/something, so the actual part where files are created is /tmp/something/usr/local/bin/python but strings embedded in the programs/libraries/docs look like /usr/local/bin/python so that they’re good at runtime.
(don’t ask me which of the three values corresponds to DESTDIR)
(edit: better replies at #17634 (comment))
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.
DESTDIR is the make equivalent of specifying --root
What is the status of this PR? |
Not sure. The OP, @ZackerySpytz might no longer be active (there was a comment suggesting that above). I don't know if @erlend-aasland's concerns have been addressed - the Given that the motivation was related to Android builds of Python, and Android is now a supported platform even though this PR has had almost no interest in 6 years, I'm inclined to say that it probably isn't necessary. |
@pfmoore I've raised some concerns in my last comment a year ago (#17634 (comment)) about the changes in
I just noticed that there's and added check (https://github.com/python/cpython/pull/17634/files#diff-ecfb0d1f34501dd30722053b9b39dea9139ffdd30dfcdc0964bcb18b95e443e4R147-R148) that makes In summary, the current state of |
@JasonGantner Thanks for the summary. Given that, and the lack of any further activity from @ZackerySpytz or anyone else interested in taking this PR forward, I'm going to close the PR. If anyone wants this functionality, they can create a new PR, either building on this one, or starting from scratch. I understand the need for the functionality, and would support someone working on this, but it doesn't seem to be a simple change, so it needs someone able and willing to do the necessary work. |
When cross-compiling, the local Python interpreter that is used to run `ensurepip` may not have the same value of `sys.prefix` as the value of the 'prefix' variable that is set in the Makefile. With the following values used to install Python locally for a later copy to the files hierarchy owned by the 'termux' application on an Android device: DESTDIR=/tmp/android prefix=/data/data/com.termux/files/usr/local 'make install' causes ensurepip to install pip in $(DESTDIR)/usr/local instead of the expected $(DESTDIR)/$(prefix) where is installed the standard library. The attached patch fixes the problem. The patch was implemented assuming that pip uses distutils for the installation (note that setup.py also uses the --prefix option in the Makefile), but I know nothing about pip so forgive me if the patch is wrong and please just assume it is just a way to demonstrate the problem. Fixes: python#75229 Fixes: https://bugs.python.org/issue31046 Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com> Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com> Co-authored-by: Zackery Spytz <zspytz@gmail.com> References: python#17634 Signed-off-by: Matěj Cepl <mcepl@cepl.eu>
When cross-compiling, the local Python interpreter that is used to run `ensurepip` may not have the same value of `sys.prefix` as the value of the 'prefix' variable that is set in the Makefile. With the following values used to install Python locally for a later copy to the files hierarchy owned by the 'termux' application on an Android device: DESTDIR=/tmp/android prefix=/data/data/com.termux/files/usr/local 'make install' causes ensurepip to install pip in $(DESTDIR)/usr/local instead of the expected $(DESTDIR)/$(prefix) where is installed the standard library. The attached patch fixes the problem. The patch was implemented assuming that pip uses distutils for the installation (note that setup.py also uses the --prefix option in the Makefile), but I know nothing about pip so forgive me if the patch is wrong and please just assume it is just a way to demonstrate the problem. Fixes: python#75229 Fixes: https://bugs.python.org/issue31046 Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com> Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com> Co-authored-by: Zackery Spytz <zspytz@gmail.com> References: python#17634 Signed-off-by: Matěj Cepl <mcepl@cepl.eu>
When using `python -m ensurepip` with the `--root` option for staged installations, the generated pip script contained an incorrect shebang that pointed into the staging directory. This made the installation unusable once the staging directory was removed. This commit fixes the issue by using the internal pip `--executable` option to force the shebang to point to the correct, final interpreter path. It also corrects related pathing issues: - Removes the check that incorrectly disallowed using --root and --prefix together. - Defaults the installation prefix to `/` when --root is used alone, ensuring installation occurs at the base of the staging directory. References: python#17634 (comment) Signed-off-by: Matěj Cepl <mcepl@cepl.eu>
Replaced by #135488, let’s make this ship sail! |
Co-Authored-By: Xavier de Gaye xdegaye@gmail.com
https://bugs.python.org/issue31046