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

Account for PEP668 in pip invocations #627

Merged
merged 5 commits into from
Feb 2, 2024
Merged

Conversation

Shrews
Copy link
Contributor

@Shrews Shrews commented Oct 20, 2023

  • Set the PIP_BREAK_SYSTEM_PACKAGES environment variable anywhere pip is in use to account for PEP668 which would change pip to not allow us to install in the system environment for newer versions of pip.
  • Adds new pip_install target script to encapsulate the pip install logic. This will be called to install pip in the base image, and thus inherited by the other images. For v1 EE schemas, we always copy this script to the builder image and call it since it will be separate from base. For v2, we only copy pip_install to the builder image and call it when one is defined.
  • Remove ensurepip calls from inside target scripts so we can control pip installation external to those scripts. Also, those calls are unnecessary since we install pip in the base image (caveat: see the builder image hoops outlined in previous step).
  • Add new skip_pip_install option to EE schema to manage pip installation.
  • BONUS fix for issue ansible-builder version 3 has broken compatibility with schema version 1 #646 which was preventing adding a somewhat decent test for this feature.

Fixes #604
Fixes #646

@Shrews Shrews force-pushed the issue604 branch 2 times, most recently from c132dde to fe12d56 Compare October 20, 2023 18:42
Set the PIP_BREAK_SYSTEM_PACKAGES environment variable anywhere
pip is in use to account for PEP668 which would change pip to not
allow us to install in the system environment for newer versions of
pip.
@Shrews Shrews marked this pull request as ready for review October 20, 2023 19:00
@Shrews Shrews requested a review from a team as a code owner October 20, 2023 19:00
@Shrews Shrews added the needs_triage New item that needs to be triaged label Oct 24, 2023
@nitzmahone
Copy link
Member

This looks good, but was just playing with it while testing some other core PEP668 work Matt C and I were doing and tripped over a nasty problem that's only apparent when actually running in a PEP668-marked environment: it doesn't look like ensurepip can respect the envvar, and will thus refuse to work at all.

It doesn't look like any RHELish OSs are marking things yet (only recent Debian/Ubuntu), but it's easy to fake it up for testing using the following as your base image:

FROM fedora:39
RUN touch /usr/lib64/python3.12/EXTERNALLY-MANAGED

Build that locally, then use the resultant image as your base in a vanilla EE with the default Python settings, and it'll 💣 hard on ensurepip. It explicitly blocks inheritance of all PIP_ envvars when pip is launched to bootstrap itself, and there's no other apparent way to pass that through.

So either we'll need to look up, zap (and possibly re-create?) the marker file, not use ensurepip, or ... something else.

@Shrews
Copy link
Contributor Author

Shrews commented Nov 6, 2023

Well that's a bunch of poo... For reference, this builder EE is enough show the error:

---
version: 3

images:
  base_image:
    name: docker.io/library/fedora:39

additional_build_steps:
  prepend_base:
    - RUN touch /usr/lib64/python3.12/EXTERNALLY-MANAGED

@sivel
Copy link
Member

sivel commented Nov 6, 2023

I have no idea if this is intended behavior, but you can also bypass EXTERNALLY-MANAGED by just supplying --root, with either pip or ensurepip. If you set --root / that works just like we do now.

[root@8f8a1523eade /]# touch /usr/lib64/python3.12/EXTERNALLY-MANAGED
[root@8f8a1523eade /]# python3 -m ensurepip
error: externally-managed-environment

× This environment is externally managed
╰─> The Python environment under /usr is managed externally, and may not be
    manipulated by the user. Please use specific tooling from the distributor of
    the Python installation to interact with this environment instead.


note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.
[SNIP]
[root@8f8a1523eade /]# python3 -m ensurepip --root /
Looking in links: /tmp/tmpn6uk_n0x
Processing /tmp/tmpn6uk_n0x/pip-23.2.1-py3-none-any.whl
Installing collected packages: pip
Successfully installed pip-23.2.1
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv

@sivel
Copy link
Member

sivel commented Nov 6, 2023

Ok, using --root is expected to bypass the check:

https://github.com/pypa/pip/blob/9685f64fe8c78e1e39cd9b32e5615f42e0a01f1c/src/pip/_internal/commands/install.py#L270-L274

        # Check whether the environment we're installing into is externally
        # managed, as specified in PEP 668. Specifying --root, --target, or
        # --prefix disables the check, since there's no reliable way to locate
        # the EXTERNALLY-MANAGED file for those cases. An exception is also
        # made specifically for "--dry-run --report" for convenience.

Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

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

LGTM

@Akasurde Akasurde removed the needs_triage New item that needs to be triaged label Nov 15, 2023
@nitzmahone
Copy link
Member

I'm going to verify one more thing to be sure, but after messing around with ensurepip --root /, I think it's basically working for us by accident. I didn't really like adding ensurepip in the first place, but it's kind of a necessary evil for common vanilla base images that wouldn't require users to always manually add a prepend_base and figure out how to get pip in there.

Much as I hate to say it, I think we're going to want to hide the ensurepip stuff behind a reusable script indirection where we can do a little more work to make sure we've got a pip available that can do what we want. The risk is that that script indirection becomes another place where everyone wants to hang "magic" to silently make pip work for $random_base_image_with_unique_busted_pip, but I think it's preferable to the game of distributed whac-a-mole I can see this one becoming down the road 😆 .

@Shrews
Copy link
Contributor Author

Shrews commented Jan 22, 2024

Per internal discussion, going to move the ensurepip call to a new target script to encapsulate future changes that might be needed for non-RHEL distros, and add a flag to disable the call.

@github-actions github-actions bot added the docs Changes to documentation label Jan 26, 2024
@Shrews Shrews marked this pull request as draft January 29, 2024 16:50
@Shrews Shrews force-pushed the issue604 branch 4 times, most recently from b280399 to fe2b956 Compare February 1, 2024 19:13
@Shrews Shrews marked this pull request as ready for review February 1, 2024 19:25
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Might be able to move/remove the re-declarations of ENV PIP_BREAK_SYSTEM_PACKAGES=1 (one looks unnecessary, the other only for pre-v3), but LGTM!

src/ansible_builder/containerfile.py Outdated Show resolved Hide resolved
@Shrews Shrews merged commit 7aa4d41 into ansible:devel Feb 2, 2024
14 checks passed
@Shrews Shrews deleted the issue604 branch February 2, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Changes to documentation
Projects
None yet
4 participants