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

Pacman: Add support for install reason #4956

Merged
merged 20 commits into from
Jul 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/4956-pacman-install-reason.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- pacman - added parameters ``reason`` and ``reason_for`` to set/change the install reason of packages (https://github.com/ansible-collections/community.general/pull/4956).
96 changes: 88 additions & 8 deletions plugins/modules/packaging/os/pacman.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,22 @@
default:
type: str

reason:
description:
- The install reason to set for the packages.
choices: [ dependency, explicit ]
type: str
felixfontein marked this conversation as resolved.
Show resolved Hide resolved
version_added: 5.4.0

reason_for:
description:
- Set the install reason for C(all) packages or only for C(new) packages.
- In case of C(state=latest) already installed packages which will be updated to a newer version are not counted as C(new).
default: new
choices: [ all, new ]
type: str
felixfontein marked this conversation as resolved.
Show resolved Hide resolved
version_added: 5.4.0

notes:
- When used with a C(loop:) each package will be processed individually,
it is much more efficient to pass the list directly to the I(name) option.
Expand Down Expand Up @@ -223,6 +239,20 @@
name: baz
state: absent
force: yes

- name: Install foo as dependency and leave reason untouched if already installed
community.general.pacman:
name: foo
state: present
reason: dependency
reason_for: new

- name: Run the equivalent of "pacman -S --asexplicit", mark foo as explicit and install it if not present
community.general.pacman:
name: foo
state: present
reason: explicit
reason_for: all
"""

import shlex
Expand Down Expand Up @@ -331,7 +361,14 @@ def run(self):
def install_packages(self, pkgs):
pkgs_to_install = []
pkgs_to_install_from_url = []
pkgs_to_set_reason = []
for p in pkgs:
if self.m.params["reason"] and (
p.name not in self.inventory["pkg_reasons"]
or self.m.params["reason_for"] == "all"
and self.inventory["pkg_reasons"][p.name] != self.m.params["reason"]
):
pkgs_to_set_reason.append(p.name)
if p.source_is_URL:
# URL packages bypass the latest / upgradable_pkgs test
# They go through the dry-run to let pacman decide if they will be installed
Expand All @@ -344,7 +381,7 @@ def install_packages(self, pkgs):
):
pkgs_to_install.append(p)

if len(pkgs_to_install) == 0 and len(pkgs_to_install_from_url) == 0:
if len(pkgs_to_install) == 0 and len(pkgs_to_install_from_url) == 0 and len(pkgs_to_set_reason) == 0:
self.exit_params["packages"] = []
self.add_exit_infos("package(s) already installed")
return
Expand Down Expand Up @@ -377,8 +414,13 @@ def _build_install_diff(pacman_verb, pkglist):
continue
name, version = p.split()
if name in self.inventory["installed_pkgs"]:
before.append("%s-%s" % (name, self.inventory["installed_pkgs"][name]))
after.append("%s-%s" % (name, version))
before.append("%s-%s-%s" % (name, self.inventory["installed_pkgs"][name], self.inventory["pkg_reasons"][name]))
if name in pkgs_to_set_reason:
after.append("%s-%s-%s" % (name, version, self.m.params["reason"]))
elif name in self.inventory["pkg_reasons"]:
after.append("%s-%s-%s" % (name, version, self.inventory["pkg_reasons"][name]))
else:
after.append("%s-%s" % (name, version))
to_be_installed.append(name)

return (to_be_installed, before, after)
Expand All @@ -398,7 +440,7 @@ def _build_install_diff(pacman_verb, pkglist):
before.extend(b)
after.extend(a)

if len(installed_pkgs) == 0:
if len(installed_pkgs) == 0 and len(pkgs_to_set_reason) == 0:
# This can happen with URL packages if pacman decides there's nothing to do
self.exit_params["packages"] = []
self.add_exit_infos("package(s) already installed")
Expand All @@ -411,9 +453,11 @@ def _build_install_diff(pacman_verb, pkglist):
"after": "\n".join(sorted(after)) + "\n" if after else "",
}

changed_reason_pkgs = [p for p in pkgs_to_set_reason if p not in installed_pkgs]

if self.m.check_mode:
self.add_exit_infos("Would have installed %d packages" % len(installed_pkgs))
self.exit_params["packages"] = sorted(installed_pkgs)
self.add_exit_infos("Would have installed %d packages" % (len(installed_pkgs) + len(changed_reason_pkgs)))
self.exit_params["packages"] = sorted(installed_pkgs + changed_reason_pkgs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only case, where the returned list of packages is sorted.
In my opinion, the returned list should be identical to the one returned if check mode is disabled.
The sorting was introduced in this commit: c28abd8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jraby Is there any reason why you only sorted the package list in check mode and not in normal mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was an oversight on my part.
The idea of the sort was to make it easier to find a package, but maybe having the output in the same order as specified in the tasks would make more sense? (their dependencies would be sprinkled throughout however)

But I don't care either way as long as check vs normal returns the same order. (the current situation makes no sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As restoring the original order would require some effort and deciding over a strategy for dependencies and just combining the lists would result in an order hard to understand for outsiders, I decided to go for sorting in dfbd46d, especially as the diff is also sorted.

In the end, this may be irrelevant, since most users probably won't even look at the list and the other users might be experienced enough to use ansible's sort, intersect or difference feature to automatically get the desired results.

return

# actually do it
Expand All @@ -430,8 +474,22 @@ def _install_packages_for_real(pacman_verb, pkglist):
if pkgs_to_install_from_url:
_install_packages_for_real("--upgrade", pkgs_to_install_from_url)

self.exit_params["packages"] = installed_pkgs
self.add_exit_infos("Installed %d package(s)" % len(installed_pkgs))
# set reason
if pkgs_to_set_reason:
cmd = [self.pacman_path, "--noconfirm", "--database"]
if self.m.params["reason"] == "dependency":
cmd.append("--asdeps")
else:
cmd.append("--asexplicit")
cmd.extend(pkgs_to_set_reason)

rc, stdout, stderr = self.m.run_command(cmd, check_rc=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not have

Suggested change
rc, stdout, stderr = self.m.run_command(cmd, check_rc=False)
rc, stdout, stderr = self.m.run_command(cmd, check_rc=True)

and remove the explicit rc check below?

Copy link
Contributor Author

@Minei3oat Minei3oat Jul 31, 2022

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess all these places should be changed then. But we can also do that later.

if rc != 0:
self.fail("Failed to install package(s)", cmd=cmd, stdout=stdout, stderr=stderr)
self.add_exit_infos(stdout=stdout, stderr=stderr)

self.exit_params["packages"] = sorted(installed_pkgs + changed_reason_pkgs)
self.add_exit_infos("Installed %d package(s)" % (len(installed_pkgs) + len(changed_reason_pkgs)))

def remove_packages(self, pkgs):
# filter out pkgs that are already absent
Expand Down Expand Up @@ -630,6 +688,7 @@ def _build_inventory(self):
"available_pkgs": {pkgname: version},
"available_groups": {groupname: set(pkgnames)},
"upgradable_pkgs": {pkgname: (current_version,latest_version)},
"pkg_reasons": {pkgname: reason},
}

Fails the module if a package requested for install cannot be found
Expand Down Expand Up @@ -722,12 +781,31 @@ def _build_inventory(self):
rc=rc,
)

pkg_reasons = {}
dummy, stdout, dummy = self.m.run_command([self.pacman_path, "--query", "--explicit"], check_rc=True)
# Format of a line: "pacman 6.0.1-2"
for l in stdout.splitlines():
l = l.strip()
if not l:
continue
pkg = l.split()[0]
pkg_reasons[pkg] = "explicit"
dummy, stdout, dummy = self.m.run_command([self.pacman_path, "--query", "--deps"], check_rc=True)
# Format of a line: "pacman 6.0.1-2"
for l in stdout.splitlines():
l = l.strip()
if not l:
continue
pkg = l.split()[0]
pkg_reasons[pkg] = "dependency"

return dict(
installed_pkgs=installed_pkgs,
installed_groups=installed_groups,
available_pkgs=available_pkgs,
available_groups=available_groups,
upgradable_pkgs=upgradable_pkgs,
pkg_reasons=pkg_reasons,
)


Expand All @@ -748,6 +826,8 @@ def setup_module():
upgrade_extra_args=dict(type="str", default=""),
update_cache=dict(type="bool"),
update_cache_extra_args=dict(type="str", default=""),
reason=dict(type="str", choices=["explicit", "dependency"]),
reason_for=dict(type="str", default="new", choices=["new", "all"]),
),
required_one_of=[["name", "update_cache", "upgrade"]],
mutually_exclusive=[["name", "upgrade"]],
Expand Down
1 change: 1 addition & 0 deletions tests/integration/targets/pacman/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@
- include: 'remove_nosave.yml'
- include: 'update_cache.yml'
- include: 'locally_installed_package.yml'
- include: 'reason.yml'
97 changes: 97 additions & 0 deletions tests/integration/targets/pacman/tasks/reason.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
---
- vars:
reg_pkg: ed
url_pkg: lemon
file_pkg: hdparm
file_pkg_path: /tmp/pkg.zst
extra_pkg: core/sdparm
extra_pkg_outfmt: sdparm
block:
- name: Make sure that test packages are not installed
pacman:
name:
- '{{reg_pkg}}'
- '{{url_pkg}}'
- '{{file_pkg}}'
- '{{extra_pkg}}'
state: absent

- name: Get URL for {{url_pkg}}
command:
cmd: pacman --sync --print-format "%l" {{url_pkg}}
register: url_pkg_url

- name: Get URL for {{file_pkg}}
command:
cmd: pacman --sync --print-format "%l" {{file_pkg}}
register: file_pkg_url
- name: Download {{file_pkg}} pkg
get_url:
url: '{{file_pkg_url.stdout}}'
dest: '{{file_pkg_path}}'

- name: Install packages from mixed sources as dependency (check mode)
pacman:
name:
- '{{reg_pkg}}'
- '{{url_pkg_url.stdout}}'
- '{{file_pkg_path}}'
reason: dependency
check_mode: True
register: install_1

- name: Install packages from mixed sources as explicit
pacman:
name:
- '{{reg_pkg}}'
- '{{url_pkg_url.stdout}}'
- '{{file_pkg_path}}'
reason: explicit
register: install_2

- name: Install packages from mixed sources with new packages being installed as dependency - (idempotency)
pacman:
name:
- '{{reg_pkg}}'
- '{{url_pkg_url.stdout}}'
- '{{file_pkg_path}}'
reason: dependency
register: install_3

- name: Install new package with already installed packages from mixed sources as dependency
pacman:
name:
- '{{reg_pkg}}'
- '{{url_pkg_url.stdout}}'
- '{{file_pkg_path}}'
- '{{extra_pkg}}'
reason: dependency
register: install_4

- name: Set install reason for all packages to dependency
pacman:
name:
- '{{reg_pkg}}'
- '{{url_pkg_url.stdout}}'
- '{{file_pkg_path}}'
- '{{extra_pkg}}'
reason: dependency
reason_for: all
register: install_5

- assert:
that:
- install_1 is changed
- install_1.msg == 'Would have installed 3 packages'
- install_1.packages|sort() == [reg_pkg, url_pkg, file_pkg]|sort()
- install_2 is changed
- install_2.msg == 'Installed 3 package(s)'
- install_2.packages|sort() == [reg_pkg, url_pkg, file_pkg]|sort()
- install_3 is not changed
- install_3.msg == 'package(s) already installed'
- install_4 is changed
- install_4.msg == 'Installed 1 package(s)'
- install_4.packages == [extra_pkg_outfmt]
- install_5 is changed
- install_5.msg == 'Installed 3 package(s)'
- install_5.packages|sort() == [reg_pkg, url_pkg, file_pkg]|sort()
43 changes: 42 additions & 1 deletion tests/unit/plugins/modules/packaging/os/test_pacman.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,19 @@ def get_bin_path(self, arg, required=False):
"upgradable_pkgs": {
"sqlite": VersionTuple(current="3.36.0-1", latest="3.37.0-1"),
},
"pkg_reasons": {
"file": "explicit",
"filesystem": "explicit",
"findutils": "explicit",
"gawk": "explicit",
"gettext": "explicit",
"grep": "explicit",
"gzip": "explicit",
"pacman": "explicit",
"pacman-mirrorlist": "dependency",
"sed": "explicit",
"sqlite": "explicit",
},
}

empty_inventory = {
Expand All @@ -108,6 +121,7 @@ def get_bin_path(self, arg, required=False):
"installed_groups": {},
"available_groups": {},
"upgradable_pkgs": {},
"pkg_reasons": {},
}


Expand Down Expand Up @@ -255,6 +269,27 @@ def test_fail(self, mock_empty_inventory):
""",
"",
),
( # pacman --query --explicit
0,
"""file 5.41-1
filesystem 2021.11.11-1
findutils 4.8.0-1
gawk 5.1.1-1
gettext 0.21-1
grep 3.7-1
gzip 1.11-1
pacman 6.0.1-2
sed 4.8-1
sqlite 3.36.0-1
""",
"",
),
( # pacman --query --deps
0,
"""pacman-mirrorlist 20211114-1
""",
"",
),
],
None,
),
Expand All @@ -272,6 +307,8 @@ def test_fail(self, mock_empty_inventory):
"",
"warning: config file /etc/pacman.conf, line 34: directive 'TotalDownload' in section 'options' not recognized.",
),
(0, "", ""),
(0, "", ""),
],
None,
),
Expand All @@ -288,6 +325,8 @@ def test_fail(self, mock_empty_inventory):
"partial\npkg\\nlist",
"some warning",
),
(0, "", ""),
(0, "", ""),
],
AnsibleFailJson,
),
Expand Down Expand Up @@ -375,6 +414,8 @@ def test_update_db_check(self, mock_empty_inventory):
(["pacman", "--query", "--groups"], {'check_rc': True}, 0, '', ''),
(["pacman", "--sync", "--groups", "--groups"], {'check_rc': True}, 0, '', ''),
(["pacman", "--query", "--upgrades"], {'check_rc': False}, 0, '', ''),
(["pacman", "--query", "--explicit"], {'check_rc': True}, 0, 'foo 1.0.0-1', ''),
(["pacman", "--query", "--deps"], {'check_rc': True}, 0, '', ''),
],
False,
),
Expand Down Expand Up @@ -843,7 +884,7 @@ def test_op_packages_nothing_to_do(
],
"state": "present",
},
["sudo", "somepackage", "otherpkg"],
["otherpkg", "somepackage", "sudo"],
[
Package("sudo", "sudo"),
Package("grep", "grep"),
Expand Down