Changes

Removes requirements_parser Python dependency.
Dependency sanitization functionality is removed.
- The --sanitize option still exists, but is hidden, and it does nothing.
- EXCLUDE_REQUIREMENTS only applied to pip before, simply because sanitize_requirements was only ever used for pip, but it now applies to system as well.
- Most Python requirements.txt entries are simply pushed down to pip (-r still causes recursive inclusion inline).
Ability to exclude top-level Python or system dependencies from collections.
- New exclude schema keyword is added to version 3 EE format and requires ansible-builder version 3.1 or higher. Using an older builder version simply errors.
- Only supports exclusion of first level deps, does not offer a way to exclude deps of deps.
- Exclusion matching is done with the simple name of the requirement (no versions, etc.).
- Exclusions can be either straight string matching, or an advanced form using regular expressions.

Bug Fixes

Comment parsing fix to no longer strip the # anchor from requirement URLs (e.g., git+https://git.repo/some_pkg.git#egg=SomePackage)

Example

---
version: 3

images:
  base_image:
    name: quay.io/centos/centos:stream9

dependencies:
  python_interpreter:
    package_system: python3.11
    python_path: /usr/bin/python3.11

  ansible_core:
    package_pip: ansible-core

  ansible_runner:
    package_pip: ansible-runner

  galaxy:
    collections:
      - name: community.docker
      - name: ansible.netcommon

  exclude:
    python:
      - docker
    system:
      - python3-Cython
    all_from_collection:
      - a.b
      - c.d

TODO:

[x] python excludes should work
[x] excludes should not apply to user deps
[x] system excludes should work
[x] a means to exclude all deps from a collection (all_from_collection or from_collection?)
[x] A docs note that Python requirements in collections should be limited to features defined by PEP508, and that any deviation from that will be considered undefined/unsupported behavior, even if it happens to work today.

sivel commented 5 months ago

Here is a list of related issues:

Fixes:

Impacts:

https://github.com/ansible/ansible-builder/issues/562

kurokobo commented 5 months ago

Hi, thanks for working on this! Sorry for adding a comment before making enough test, but since this PR at this moment simply uses \n to separate lines for requirements.txt for collections (in pip_file_data) and process them line by line, I wonder if the collections' requirements.txt would behave unexpectedly if it contained line continuations with backslashes.

Example collection: https://github.com/PaloAltoNetworks/pan-os-ansible/blob/develop/requirements.txt
Docs about line continuations in requirement files: https://pip.pypa.io/en/stable/reference/requirements-file-format/#line-continuations

sivel commented 5 months ago

I wonder if the collections' requirements.txt would behave unexpectedly if it contained line continuations with backslashes

@kurokobo that is correct. We discussed this when architecting the functionality, and should a package that is excluded have a backslash with a newline, it will likely cause problems.

We don't plan to add any specific functionality to permit or deny newlines, but we do intend to add documentation that states that it is in the best interest of everyone for collection maintainers, and EE creators to use pep508 formatted requirements files. We may be going so far to add linting rules, potentially via galaxy-importer to warn when a collection defines non-pep508 formatted requirements files.

kurokobo commented 5 months ago

@sivel Thanks for providing detailed background! I have no objection to the policy, but I want to know details a bit.

If we strictly follow pep508, even any comments (#) would not be allowed. But this PR does not remove an implementation that intentionally ignores comments; is it a policy that requires strictly forrowing pep508, but allows comments as an exception? Or is my understanding incorrect?

F.Y.I., to support line continuations, we need just one modification (I don't think it will be adopted, but I'll write it down as I think of it😅 ).

diff --git a/src/ansible_builder/_target_scripts/introspect.py b/src/ansible_builder/_target_scripts/introspect.py
index 553cf81..c32d121 100644
--- a/src/ansible_builder/_target_scripts/introspect.py
+++ b/src/ansible_builder/_target_scripts/introspect.py
@@ -26,7 +26,7 @@ def read_req_file(path):

 def pip_file_data(path):
-    pip_content = read_req_file(path)
+    pip_content = read_req_file(path).replace('\\\n', '')

     pip_lines = []
     for line in pip_content.split('\n'):

Shrews commented 5 months ago

@sivel Thanks for providing detailed background! I have no objection to the policy, but I want to know details a bit.

If we strictly follow pep508, even any comments (#) would not be allowed. But this PR does not remove an implementation that intentionally ignores comments; is it a policy that requires strictly forrowing pep508, but allows comments as an exception? Or is my understanding incorrect?

Comments will be allowed. Any non-pep508 compliant entries are going to be passed through without change, unchecked and unverified. We plan to update the docs that using such non-compliant entries will be considered undefined/unsupported behavior.

Shrews commented 4 months ago

Removed association with issue #364. This PR would pass through unrecognized (and potentially invalid) requirements.

djgoosen commented 3 weeks ago

Hi there, unfortunately #664 appears to be breaking numerous previously known-good custom EE builds that I am seeing in a variety of different user environments recently, even those which had already been migrated to v3 spec files, etc. While I'm sure it was well-intentioned, if even a relatively simple custom EE for something like Palo Alto will no longer build if the paloaltonetworks.panos collection itself is given in its galaxy requirements alongside its underlying python requirements (pan-python and pan-os-python), which certainly appears to be the case in ansible-builder 3.1.0, then it probably should be reviewed.

Based on the changelog, it seems like it was anticipated that it might "cause build issues in images with older versions of pip that cannot handle duplicate requirement entries". However, even on pip latest, this change (to expectations for the Ansible Builder community on how any duplicate Python dependencies will be handled and resolved at build time) is breaking things, and evidently without any definitive, prescriptive guidance so far on what exactly everyone should do going forward.

(ab300) $ pip3 --version
pip 24.2 from /Users/x/ab300/lib/python3.11/site-packages/pip (python 3.11)
(ab300) $ ansible-builder build -t palo-ee
Running command:
  podman build -f context/Containerfile -t palo-ee context
Complete! The build context can be found at: /Users/x/palo-ee/context
(ab300) $ ansible-builder --version
3.0.0
(ab300) $ deactivate
$ source ab310/bin/activate
(ab310) $ ansible-builder --version
3.1.0
(ab310) $ ansible-builder build -t palo-ee
Running command:
  podman build -f context/Containerfile -t palo-ee context
...showing last 20 lines of output...
++ export PATH
++ '[' -n '' ']'
++ '[' -z '' ']'
++ _OLD_VIRTUAL_PS1=
++ PS1='(venv) '
++ export PS1
++ '[' -n /bin/bash -o -n '' ']'
++ hash -r
+ '[' -f /tmp/src/upper-constraints.txt ']'
+ [[ -n '' ]]
+ install_wheels
+ '[' -f /tmp/src/build-requirements.txt ']'
+ '[' -f setup.py ']'
+ '[' -f /tmp/src/requirements.txt ']'
+ '[' '!' -f /output/requirements.txt ']'
+ /usr/bin/python3 -m pip install --cache-dir=/output/wheels -r /tmp/src/requirements.txt
WARNING: Running pip install with root privileges is generally not a good idea. Try `python3 -m pip install --user` instead.
ERROR: Double requirement given: pan-os-python (from -r /tmp/src/requirements.txt (line 13)) (already in pan-os-python==1.8.0 (from -r /tmp/src/requirements.txt (line 1)), name='pan-os-python')
Error: building at STEP "RUN /output/scripts/assemble": while running runtime: exit status 1

An error occurred (rc=1), see output line(s) above for details.
(ab310) $

Shrews commented 3 weeks ago

Hello @djgoosen. In the future, it's better to open a new issue than to comment on a closed PR since we may not always notice these types of comments.

The 3.1 release was sort of a big update with some very necessary changes that were required in order to progress the project forward and eliminate some unmaintained external libraries. Backward compatibility with EEs that work with version 3.0 was never guaranteed, thus the new Y-release version.

In your particular case, upgrading pip within your EE will likely fix your problem. In your PR description, you are showing the pip version information for software outside of the container image, but it is the version of pip within the container image that matters (that is where Python packages are installed). You can either use a more recent base image with a newer pip, or use additional_build_steps to upgrade the version of pip within the container image.

If that does not resolve your issue, you also have the option of using the new exclude feature in 3.1 to ignore any dependencies from collections that may be causing you issues.

If both of those solutions fail for you, please open a new issue and provide the EE file where you are seeing the problem.

ansible / ansible-builder

Expose a way to exclude dependencies #664

Changes

Bug Fixes

Example