Portage: remove gentoolkit requirement for portage's python module #3870

0xdc · 2021-12-08T13:41:38Z

SUMMARY

The package app-portage/gentoolkit is not a requirement for a working Gentoo/portage installation (i.e. the package is not provided in any system set). This prevents those hosts from using an otherwise usable module, and forces administrators who wish to use it to install an extra package (by some other method).

Switch the portage ansible module to use the portage python module for checking if a package is installed. This allows ansible to work on more hosts without workarounds.

Since a working python installation is required for portage to work, this does not add an extra, optional dependency.

ISSUE TYPE

Bug Fix Pull Request

COMPONENT NAME

portage

ADDITIONAL INFORMATION

Before this change:

host | FAILED! => {
    "ansible_facts": {
        "discovered_interpreter_python": "/usr/bin/python3.9"
    },
    "changed": false,
    "msg": "Failed to find required executable \"equery\" in paths: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/bin"
}

After this change:

host | CHANGED => {
    "ansible_facts": {
        "discovered_interpreter_python": "/usr/bin/python3.9"
    },
    "changed": true,
    "cmd": [
        "/usr/bin/emerge",
        "--noreplace",
        "--update",
        "--ask=n",
        "--pretend",
        "gentoolkit"
    ],
    "msg": "Packages would be installed.",
    "rc": 0,
    "stderr": "",
    "stderr_lines": [],
    "stdout": "\nThese are the packages that would be merged, in order:\n\nCalculating dependencies  .... done!\n[binary  N     ] app-portage/gentoolkit-0.5.1-r1-1  USE=\"-test\" PYTHON_TARGETS=\"python3_9 (-pypy3) -python3_8 (-python3_10)\" \n",
    "stdout_lines": [
        "",
        "These are the packages that would be merged, in order:",
        "",
        "Calculating dependencies  .... done!",
        "[binary  N     ] app-portage/gentoolkit-0.5.1-r1-1  USE=\"-test\" PYTHON_TARGETS=\"python3_9 (-pypy3) -python3_8 (-python3_10)\" "
    ]
}

The package `app-portage/gentoolkit` is not a requirement for a working Gentoo/portage installation (i.e. the package is not provided in any `system` set). This prevents those hosts from using an otherwise usable module, and forces administrators who wish to use it to install an extra package (by some other method). Switch the portage ansible module to use the portage python module for checking if a package is installed. This allows ansible to work on more hosts without workarounds. Since a working python installation is required for portage to work, this does not add an extra, optional dependency.

With the inclusion of the portage module, we can access some constants and runtime variables. Unhardcode the path the the world sets file. This also makes it possible to access world sets in alternative installations when $ROOT is set.

ansibullbot · 2021-12-08T13:46:44Z

cc @Tatsh @wltjr
click here for bot help

felixfontein · 2021-12-08T19:29:59Z

Thanks for your contribution!

Please note that right now this is a breaking change and cannot be merged as-is. The reason is that right now, the Ansible module also works when being executed with a Python interpreter on the machine which does not have the portage Python module installed. With this PR, having the portage Python module installed for the same Python this module is executed with is a hard requirement.

The standard approach to handle such situations is to make the module work in both situations:

When the portage Python module is available, use it.
If it is not available, fall back to calling the CLI tool.

(Or even better: offer an option which configures the behavior, with default value auto for auto-detection; that way if someone experiences problems with the new approach, they can force the module to fall back to the old behavior.)

Also, please note that every code change requires a changelog fragment. It should be a minor_changes fragment, since this would be a new feature (if implemented as I wrote above).

wltjr · 2021-12-15T02:45:56Z

@0xdc nice work! Thank you for seeing it through and making the necessary changes to get it passing all tests and committed!

felixfontein · 2021-12-15T06:57:07Z

changelogs/fragments/3870-portage-prefer-pymodule.yml

+  - portage - Make the module work on systems without the non-system package
+    gentoolkit. If possible, use the ansible interpreter's 'portage' module.
+    (https://github.com/ansible-collections/community.general/pull/3870)


Suggested change

- portage - Make the module work on systems without the non-system package

gentoolkit. If possible, use the ansible interpreter's 'portage' module.

(https://github.com/ansible-collections/community.general/pull/3870)

- portage - make the module work on systems without the non-system package

gentoolkit. If possible, use the Python interpreter's ``portage`` module

(https://github.com/ansible-collections/community.general/pull/3870).

felixfontein · 2021-12-15T06:58:30Z

plugins/modules/packaging/os/portage.py

-    module.equery_path = module.get_bin_path('equery', required=True)
+    try:
+        import portage
+        module.vartree = portage.vartree()


Please don't modify module, but instead pass vartree and equery_path on as explicit function parameters.

Also, this line is probably unlikely to cause an ImportError and shouldn't be inside the try-block anyway (it could be moved to an else-block, though).

ajakk · 2021-12-27T06:46:48Z

Why is it desirable to use gentoolkit at all? What system with Portage wouldn't have the Portage Python module available? That might be fragile and confusing to users in the case that the /usr/bin/python executable is a Python implementation for which the Portage module doesn't exist.

felixfontein · 2021-12-27T08:32:18Z

@ajakk that Python module is usually just available for one Python version (the system Python). Users could use another Python version for Ansible, or use a venv which has no access to the system-wide installed Python modules. If this would be a new module it would be ok to simply assume it is run with the correct Python interpreter and that module available, but this module was working before in such non-standard environments since it didn't rely on that Python module. So just changing it would be a breaking change, which isn't accepted without a long deprecation period (and during that period both ways have to be supported anyway, i.e. what this PR is now doing).

ajakk · 2021-12-27T18:26:01Z

No, the Portage Python module is available for whatever versions of Python dev-lang/portage (on Gentoo) was built for. Currently, if you even have Python 3.10 installed, Ansible tries to use that, despite almost no one having a Portage module for Python 3.10 because that target is not a default yet. So there's already breakage.

Is there really a system on which you'd try to use Portage and also not have the Portage Python module?

Edit: To clarify, I'm asking because I do not know of any case where this might break anything, so I don't know what concern there could be of breaking things.

felixfontein · 2021-12-27T20:27:09Z

@ajakk you are for example ignoring the case where someone compiles their own Python (not from any Gentoo provided sources). Or does Gentoo have some magic that automatically detects any kind of Python interpreter running under it and inserts the module into it? Also there's the more common case of a virtual environment that does not include the system-wide installed modules (minimal venvs). And I'm also pretty sure that Gentoo doesn't provide a version of the Python module for every Python version that you can possibly install on it.

ajakk · 2021-12-27T22:46:08Z

I'm not ignoring anything. Hanlon's Razor.

I don't see how the way Python is installed affects anything. If Portage is installed, the Portage module is available, and we know this because the Portage module is implicitly used anyway by emerge. If I understand your concern correctly, you seem to be worried about the case that the Python version which Ansible is run under doesn't have a Portage Python module available to it. This is already happening.

Ansible uses the latest Python available, which is not necessarily the version for which a Portage module is installed. For example, I have Python 3.10 installed on some of my Gentoo systems. Ansible uses the latest Python available to it (3.10 for these systems), but the Portage module is (probably almost never) installed for Python 3.10 because the current Gentoo default is to build Python modules for Python 3.9, so Ansible does not work out of the box on such systems. So the breakage you're worried about is already happening. If a user is crazy enough to install Python outside of Portage (potentially breaking their Gentoo because the package manager is Python), I'm not sure why Ansible should try to be clever about it.

And, on a package-by-package basis, Gentoo can provide versions of Python modules for any supported Python implementation.

felixfontein · 2021-12-27T22:57:47Z

You make the assumption that the Python that's used by Ansible to execute the module (which can be configured by the user) is the same Python that's used to run emerge. That is not necessarily the case.

thesamesam · 2021-12-28T01:04:34Z

You make the assumption that the Python that's used by Ansible to execute the module (which can be configured by the user) is the same Python that's used to run emerge. That is not necessarily the case.

This problem is going to exist either way by sending across .py files and hoping that the Python interpreter chosen to run them matches the installed Python implementations for any modules (e.g. Portage) needed. Therefore, no guarantee exists that the sent .py will be able to run, it depends on what the user has set their python to be.

For now, at the very least, it should be explicitly documented (is it already?) that the set python must not be mangled. Note that by default, it'll end up matching the default Python implementation version, so it works out okay: see here. In Gentoo, we changed the behaviour somewhat recently to default to this.

An explicit shebang should be used and/or some check to verify the installed implementations for a module and bail out if they don't match (a hack to do this would be e.g. check whether /usr/lib/python-exec/python3.10/emerge exists, etc).

Raising an ImportError with a message that explains the problem would be useful. Or try to find some executable which Portage installs that you can call that gives the information you need, which sidesteps the problem (portageq, maybe).

(I'd note that considering a custom-compiled Python is out of scope in my view given we don't support that in Gentoo, not least because it wouldn't integrate with the python-exec mechanism I describe correctly.)

felixfontein · 2021-12-28T08:39:34Z

Ansible does not simply copy a Python file over and executes it. It does a lot of other things as well, like replace the placeholder shebang (

community.general/plugins/modules/packaging/os/portage.py

Line 1 in f14374c

#!/usr/bin/python

) by one referencing the Python version that the user configured, or (in case the user didn't explicitly configure one) that was discovered (https://docs.ansible.com/ansible/latest/reference_appendices/interpreter_discovery.html).

So I don't really see what you folks are arguing here.

(I'd note that considering a custom-compiled Python is out of scope in my view given we don't support that in Gentoo, not least because it wouldn't integrate with the python-exec mechanism I describe correctly.)

The user can tell Ansible to use such an interpreter on a Gentoo system. So yes, it is fully in scope here.

thesamesam · 2021-12-28T08:45:11Z

Ansible does not simply copy a Python file over and executes it. It does a lot of other things as well, like replace the placeholder shebang (f9e2e055..19a4f760 (#3951)) by one referencing the Python version that the user configured, or (in case the user didn't explicitly configure one) that was discovered (docs.ansible.com/ansible/latest/reference_appendices/interpreter_discovery.html).

I'm not an Ansible user (okay, I'm a very early beginner). I'm just offering a perspective as a Gentoo developer, I'm not trying to trivialise what Ansible does. I'm really pleased Gentoo even has a module! If I didn't value it, I'm not sure I'd be participating in the discussion 😉

Thanks for the links!

The issue still remains about needing to align them though. It doesn't seem like a particularly huge burden to just request that if someone changes their chosen Python for use with Ansible, they verify that Portage is configured correctly to work with it. That is all my point was. Seems great that Ansible already offers that degree of flexibility.

i.e. while it may be a breaking change, anyone using Gentoo and Ansible would likely be aware of the need to align things given that in the past (until very recently), the default python would just end up being something with very few or no deps installed at all.

One gets a significant benefit from this PR: not needing an external dependency.

So I don't really see what you folks are arguing here.

I'm not arguing over anything - I think you may have read a bit too much into my comment. I'm just trying to give my perspective in case it's useful. I appreciate the links and information you've given 😃

(I'd note that considering a custom-compiled Python is out of scope in my view given we don't support that in Gentoo, not least because it wouldn't integrate with the python-exec mechanism I describe correctly.)

The user can tell Ansible to use such an interpreter on a Gentoo system. So yes, it is fully in scope here.

I'm just pointing out that Gentoo doesn't consider such custom built packages outside of the package manager supported, so Ansible doesn't have to support it either. If it wants to, so be it.

(And it's therefore, from an upstream perspective, not something to constrain the common case over.)

But I did in fact offer two suggestions, which seem to fit within your parameters, to move forward rather than just "arguing" anyway.

All I really wanted to add was I felt that it was a reasonable compromise to drop complexity and not bother falling back to gentoolkit given it's kind of an unconventional use of it, and document that people should align their Python impl if neither of those two options I gave are OK.

felixfontein · 2021-12-28T08:59:25Z

The issue still remains about needing to align them though. It doesn't seem like a particularly huge burden to just request that if someone changes their chosen Python for use with Ansible, they verify that Portage is configured correctly to work with it. That is all my point was.

But that's already happen, look at the diff for this PR: https://github.com/ansible-collections/community.general/pull/3870/files#diff-5b45bc5a4dfdda389c244385dcd8de464ae022cabb925972623bf5cb79e70d20R521

With this PR, the module tries to import the portage Python module. If that succeeds, it uses it. If that fails, it falls back to using the equery executable. The equery and emerge executables are called by this module with subprocess.Popen, so they use whatever Python interpreter their shebang is referencing.

So there is no breaking change:

i.e. while it may be a breaking change, anyone using Gentoo and Ansible would likely be aware of the need to align things given that in the past (until very recently), the default python would just end up being something with very few or no deps installed at all.

The first version of this PR completely replaced using the equery executable by using the portage Python module (0346a85 and fee2b05) without any fallback to the executable. That would have been a breaking change in case the Ansible module is executed by a Python interpreter that does not have access to the portage Python module. And that's why I insisted in #3870 (comment) that it needs to fall back to the executable in case it cannot import portage.

One gets a significant benefit from this PR: not needing an external dependency.

Exactly, at least in case you told Ansible to use a Python interpreter which has access to the portage module.

The user can tell Ansible to use such an interpreter on a Gentoo system. So yes, it is fully in scope here.

I'm just pointing out that Gentoo doesn't consider such custom built packages outside of the package manager supported, so Ansible doesn't have to support it either. If it wants to, so be it.

But users can build restricted venvs that do not include system-wide libraries, so even if they use the same Python hat has the portage module installed, it won't have it available in that venv. (Assuming Gentoo puts the portage module in site-packages.)

felixfontein · 2021-12-28T09:02:25Z

All I really wanted to add was I felt that it was a reasonable compromise to drop complexity and not bother falling back to gentoolkit given it's kind of an unconventional use of it, and document that people should align their Python impl if neither of those two options I gave are OK.

We can reduce complexity in the future by properly deprecating this fallback and removing it once the deprecation period expires. But we cannot do it right now, since right now there might be users which use a Python interpreter or a venv which does not provide portage, and if we'd remove the fallback right now, it will break for these users.

thesamesam · 2021-12-28T09:04:54Z

The issue still remains about needing to align them though. It doesn't seem like a particularly huge burden to just request that if someone changes their chosen Python for use with Ansible, they verify that Portage is configured correctly to work with it. That is all my point was.

But that's already happen, look at the diff for this PR: #3870 (files)

With this PR, the module tries to import the portage Python module. If that succeeds, it uses it. If that fails, it falls back to using the equery executable. The equery and emerge executables are called by this module with subprocess.Popen, so they use whatever Python interpreter their shebang is referencing.

Got it. Thank you for explaining!

What do you think about:

Don't call equery from gentookit (I think it's currently only used for checking if an atom is installed/satisfied?)
Instead call portageq has_version / <atom>, which is provided by Portage itself?

It should have the same exit code behaviour as equery. This way, no external deps are needed even when things are misaligned.

EDIT: Ah, I didn't see your second comment. I don't mind revisiting this suggestion later then. Better to not involve too many changes at once.

felixfontein · 2021-12-28T10:33:45Z

What do you think about:
1. Don't call `equery` from gentookit (I think it's currently only used for checking if an atom is installed/satisfied?)

2. Instead call `portageq has_version / <atom>`, which is provided by Portage itself?
It should have the same exit code behaviour as equery. This way, no external deps are needed even when things are misaligned.

Sounds like a good idea. (Assuming that portageq has_version has been around for long enough.)

But this can also be done in a separate / follow-up PR; doing too much in one PR is usually no the best idea :)

webknjaz

Additionally to Felix's sentiments, here's a few more maintainability/readability comments from me:

webknjaz · 2021-12-28T19:05:57Z

plugins/modules/packaging/os/portage.py

-    cmd = '%s list %s' % (module.equery_path, atom)
+    try:
+        installed = module.vartree.dbapi.match(atom)
+        return bool(installed)


Could you move this to an else-block?

webknjaz · 2021-12-28T19:08:38Z

plugins/modules/packaging/os/portage.py

+    try:
+        # When providing ROOT via ansible enviroment, portage.root may return a
+        # portage._LegacyGlobalProxy instance. Force it to be a string.
+        world_sets_path = os.path.join(str(portage.root), portage.const.WORLD_SETS_FILE)


What part of this nested thing can actually cause a NameError? It's hard to guess, could you wrap with try-except just that, and move the rest to an else-block?

Actually portage is the thing that triggers the NameError. But it always does, since portage is not passed here. This needs to be fixed.

webknjaz · 2021-12-28T19:10:09Z

plugins/modules/packaging/os/portage.py

+        # portage._LegacyGlobalProxy instance. Force it to be a string.
+        world_sets_path = os.path.join(str(portage.root), portage.const.WORLD_SETS_FILE)
+    except NameError:
+        world_sets_path = "/var/lib/portage/world_sets"


Nit: changing the quotation style unnecessarily makes the diff harder to read, try to avoid such unrelated changes to make it easier for the reviewers to catch up with what's going on in the PR.

webknjaz · 2021-12-28T19:12:40Z

plugins/modules/packaging/os/portage.py

@@ -506,7 +515,12 @@ def main():
    )

    module.emerge_path = module.get_bin_path('emerge', required=True)
-    module.equery_path = module.get_bin_path('equery', required=True)
+    try:
+        import portage


Importing things in the middle of the control flow is an antipattern. A typical idiom used virtually everywhere is having try-except at the top of the module and then having if-else here.

webknjaz · 2021-12-28T19:14:27Z

plugins/modules/packaging/os/portage.py

-    module.equery_path = module.get_bin_path('equery', required=True)
+    try:
+        import portage
+        module.vartree = portage.vartree()


Also, this line is probably unlikely to cause an ImportError and shouldn't be inside the try-block anyway (it could be moved to an else-block, though).

felixfontein · 2022-01-06T06:26:38Z

@0xdc can you please take a look at the review comments?

0xdc · 2022-01-06T17:54:05Z

Hi Felix,

I haven't started making changes as per the reviews yet, but I did follow along with the discussions. I think that converting this module to use portageq has_version would be sufficient.

felixfontein · 2022-01-08T14:41:58Z

Ok. I hope someone will actually implement that though :)

Daniel Cordero added 2 commits December 8, 2021 13:29

portage: unhardcode world sets path

fee2b05

With the inclusion of the portage module, we can access some constants and runtime variables. Unhardcode the path the the world sets file. This also makes it possible to access world sets in alternative installations when $ROOT is set.

ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor os packaging plugins plugin (any type) labels Dec 8, 2021

This comment has been minimized.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Dec 8, 2021

portage: fallback to equery if python interpreter does not have portage

a215efe

felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 11, 2021

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Dec 13, 2021

portage: add changelog fragment

f14374c

felixfontein reviewed Dec 15, 2021

View reviewed changes

felixfontein added the backport-4 label Dec 15, 2021

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Dec 23, 2021

webknjaz reviewed Dec 28, 2021

View reviewed changes

0xdc closed this Jan 6, 2022

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 8, 2022

ajakk mentioned this pull request Oct 12, 2022

portage: drop usage of gentoolkit, add knobs for --with-bdeps, --backtrack #5349

Merged

patchback bot mentioned this pull request Oct 14, 2022

[PR #5349/2a54644c backport][stable-5] portage: drop usage of gentoolkit, add knobs for --with-bdeps, --backtrack #5365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portage: remove gentoolkit requirement for portage's python module #3870

Portage: remove gentoolkit requirement for portage's python module #3870

0xdc commented Dec 8, 2021

ansibullbot commented Dec 8, 2021

This comment has been minimized.

felixfontein commented Dec 8, 2021

wltjr commented Dec 15, 2021

felixfontein Dec 15, 2021

felixfontein Dec 15, 2021

webknjaz Dec 28, 2021

ajakk commented Dec 27, 2021

felixfontein commented Dec 27, 2021

ajakk commented Dec 27, 2021 •

edited

Loading

felixfontein commented Dec 27, 2021

ajakk commented Dec 27, 2021

felixfontein commented Dec 27, 2021

thesamesam commented Dec 28, 2021 •

edited

Loading

felixfontein commented Dec 28, 2021 •

edited

Loading

thesamesam commented Dec 28, 2021 •

edited

Loading

felixfontein commented Dec 28, 2021

felixfontein commented Dec 28, 2021

thesamesam commented Dec 28, 2021 •

edited

Loading

felixfontein commented Dec 28, 2021

webknjaz left a comment

webknjaz Dec 28, 2021

webknjaz Dec 28, 2021

felixfontein Dec 29, 2021

webknjaz Dec 28, 2021

webknjaz Dec 28, 2021

webknjaz Dec 28, 2021

felixfontein commented Jan 6, 2022

0xdc commented Jan 6, 2022

felixfontein commented Jan 8, 2022

Portage: remove gentoolkit requirement for portage's python module #3870

Portage: remove gentoolkit requirement for portage's python module #3870

Conversation

0xdc commented Dec 8, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Dec 8, 2021

This comment has been minimized.

felixfontein commented Dec 8, 2021

wltjr commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajakk commented Dec 27, 2021

felixfontein commented Dec 27, 2021

ajakk commented Dec 27, 2021 • edited Loading

felixfontein commented Dec 27, 2021

ajakk commented Dec 27, 2021

felixfontein commented Dec 27, 2021

thesamesam commented Dec 28, 2021 • edited Loading

felixfontein commented Dec 28, 2021 • edited Loading

thesamesam commented Dec 28, 2021 • edited Loading

felixfontein commented Dec 28, 2021

felixfontein commented Dec 28, 2021

thesamesam commented Dec 28, 2021 • edited Loading

felixfontein commented Dec 28, 2021

webknjaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Jan 6, 2022

0xdc commented Jan 6, 2022

felixfontein commented Jan 8, 2022

ajakk commented Dec 27, 2021 •

edited

Loading

thesamesam commented Dec 28, 2021 •

edited

Loading

felixfontein commented Dec 28, 2021 •

edited

Loading

thesamesam commented Dec 28, 2021 •

edited

Loading

thesamesam commented Dec 28, 2021 •

edited

Loading