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

feat: dnf module #377

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from
Draft

feat: dnf module #377

wants to merge 51 commits into from

Conversation

fiftydinar
Copy link
Collaborator

@fiftydinar fiftydinar commented Dec 22, 2024

Changes

Recipe format

  • Recipe format changed a bit, to satisfy the new feature of dnf flags.

Existing features code changes

  • Adding repos is now using dnf -y config-manager addrepo --from-repofile=$repo for repo URLs & repo files, while COPR repos are using native dnf -y copr enable user/project.
    COPR repos are written in separate copr: array & in simpler user/project format in recipe, instead of long URL in repos: array.
  • rpm-ostree override remove --install=$pkg is replaced with 2 operations of dnf removal & then install.
    Single operation of removal + installation of dnf doesn't currently exist, but it's not a breaking behavior to not have it.
  • rpm-ostree override replace is replaced with dnf -y distro-sync --refresh --repo $repo $packages, which is compatible with all repos.
    Adding the repository along with replacement is disabled, so it must be done before replacement in repos: array if repo doesn't exist.
  • Fixed the bug with symlinking /opt/ if it's already symlinked in both dnf & rpm-ostree module (thanks to @lorduskordus for the fix)

New features

  • dnf group removal & installation. Those are declared as group-remove & group-install array in recipe.
    They run before the packages installation.
  • Option for additional flags when doing installation or replacement of packages.
    • install-weak-dependencies: option for declaring if weak dependencies (such as Recommended) should be installed.
      It modifies the install & replace commands itself using --setopt=install_weak_deps=True/False flag, it doesn't modify the dnf config file. It defaults to true (reflecting dnf defaults).
    • skip-unavailable-packages: option for skipping unavailable packages when not available in repositories, or when not available on the system in case of replacing. Passes --skip-unavailable flag. Defaults to false.
    • skip-broken-packages: option for skipping the installation/replacement of broken packages. Passes --skip-broken flag. Defaults to false.
    • allow-erasing-packages: option for erasing/removing the problematic packages during dependency problems. Passes --allowerasing flag. Defaults to false.
  • Option for additional flags when doing removal of packages.
    • remove-unused-dependencies is true by default. If false is specified, it passes the --no-autoremove flag, which won't remove any unused dependencies during removal operation.

Issues to be closed

Fixes: #361
Fixes: #370
Fixes: #365
Fixes: #325
Fixes: #282

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

shellcheck

⚠️ [shellcheck] reported by reviewdog 🐶
Did you forget to close this double quoted string? SC1078

CLASSIC_PKGS+=("${PKG}")


📝 [shellcheck] reported by reviewdog 🐶
This is actually an end quote, but due to next char it looks suspect. SC1079

echo "Installing: ${CLASSIC_PKGS[*]}"


🚫 [shellcheck] reported by reviewdog 🐶
'(' is invalid here. Did you forget to escape it? SC1036

echo "Installing package(s) directly from URL: ${HTTPS_PKGS[*]}"


🚫 [shellcheck] reported by reviewdog 🐶
Expected 'fi' matching previously mentioned 'if'. SC1047

echo "Installing package(s) directly from URL: ${HTTPS_PKGS[*]}"


🚫 [shellcheck] reported by reviewdog 🐶
Expected 'fi'. Fix any mentioned problems and try again. SC1072

echo "Installing package(s) directly from URL: ${HTTPS_PKGS[*]}"

example: |
type: dnf
repos:
- copr: atim/starship
Copy link
Member

Choose a reason for hiding this comment

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

I think this can get confusing for users. This will be seen as an object and not a string. We could have a separate property copr that will take an array of strings which we could then iterate over and feed into the dnf copr enable command.

type: dnf
copr:
  - atim/starship
  - ryanabx/cosmic-epoch
install:
  - starship
  - cosmic-desktop

Copy link
Collaborator Author

@fiftydinar fiftydinar Dec 22, 2024

Choose a reason for hiding this comment

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

It looks less clean & more separated if it's done this way, hence why I went with copr: user/project format.

Is : making it an object or the whitespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can maybe have COPR user/project as format instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it to COPR user/project format

Copy link
Member

@gmpinder gmpinder Dec 24, 2024

Choose a reason for hiding this comment

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

I still don't think this is better. Having a copr: property and a list of COPR repos would be very clear. Encoding meaning into the spec will be easier for a user to understand how to add new COPR repos and lead to fewer mistakes like accidentally having too much whitespace between COPR and the repo path.

Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Possible misspelling: REPOS may not be assigned. Did you mean REPO? SC2153

if [[ ${#REPOS[@]} -gt 0 ]]; then

Copy link
Contributor

Choose a reason for hiding this comment

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

[shellcheck] reported by reviewdog 🐶
$/${} is unnecessary on arithmetic variables. SC2004

INSTALL_PKGS[$i]="${PKG//%OS_VERSION%/${OS_VERSION}}"

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [shellcheck] reported by reviewdog 🐶
Quote this to prevent word splitting. SC2046

rpm-ostree override remove "${REMOVE_PKGS[@]}" $(printf -- "--install=%s " "${CLASSIC_PKGS[@]}")

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [shellcheck] reported by reviewdog 🐶
Quote this to prevent word splitting. SC2046

rpm-ostree override remove "${REMOVE_PKGS[@]}" $(printf -- "--install=%s " "${CLASSIC_PKGS[@]}")

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [shellcheck] reported by reviewdog 🐶
Quote this to prevent word splitting. SC2046

rpm-ostree override remove "${REMOVE_PKGS[@]}" $(printf -- "--install=%s " "${CLASSIC_PKGS[@]}")

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [shellcheck] reported by reviewdog 🐶
Quote this to prevent word splitting. SC2046

rpm-ostree override remove "${REMOVE_PKGS[@]}" $(printf -- "--install=%s " "${CLASSIC_PKGS[@]}")

Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

rpm-ostree override replace --experimental --from "repo=copr:copr.fedorainfracloud.org:${MAINTAINER}:${REPO_NAME}" ${REPLACE_STR}

@fiftydinar fiftydinar marked this pull request as draft December 23, 2024 09:08
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
@xynydev xynydev self-assigned this Dec 23, 2024
modules/dnf/dnf.tsp Outdated Show resolved Hide resolved
modules/dnf/dnf.tsp Outdated Show resolved Hide resolved
modules/dnf/dnf.tsp Outdated Show resolved Hide resolved
modules/dnf/dnf.tsp Outdated Show resolved Hide resolved
fiftydinar and others added 4 commits December 23, 2024 22:33
Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
modules/dnf/module.yml Outdated Show resolved Hide resolved
Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
for i in "${!REPOS[@]}"; do
repo="${REPOS[$i]}"
repo="${repo//%OS_VERSION%/${OS_VERSION}}"
REPOS[$i]="${repo//[$'\t\r\n ']}"
Copy link
Contributor

Choose a reason for hiding this comment

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

[shellcheck] reported by reviewdog 🐶
$/${} is unnecessary on arithmetic variables. SC2004

modules/dnf/dnf.sh Outdated Show resolved Hide resolved
Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
Copy link
Member

@gmpinder gmpinder left a comment

Choose a reason for hiding this comment

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

I'm getting an error when running a basic install:

l/jp-desktop-nvidia:latest => #25 0.447 ============================== Start 'dnf' Module ==============================
l/jp-desktop-nvidia:latest => #25 0.531 Installing RPMs
l/jp-desktop-nvidia:latest => #25 0.531 Installing: @kde-desktop
l/jp-desktop-nvidia:latest => #25 0.578 Updating and loading repositories:
l/jp-desktop-nvidia:latest => #25 0.800  Fedora 41 openh264 (From Cisco) - x86_ 100% |   6.5 KiB/s | 989.0   B |  00m00s
l/jp-desktop-nvidia:latest => #25 1.161  Fedora 41 - x86_64                     100% |  85.6 KiB/s |  30.8 KiB |  00m00s
l/jp-desktop-nvidia:latest => #25 1.256  Fedora 41 - x86_64 - Updates Archive   100% |  36.2 KiB/s |   3.4 KiB |  00m00s
l/jp-desktop-nvidia:latest => #25 1.416  Fedora 41 - x86_64 - Updates           100% | 184.9 KiB/s |  29.4 KiB |  00m00s
l/jp-desktop-nvidia:latest => #25 1.483 Repositories loaded.
l/jp-desktop-nvidia:latest => #25 1.952 Failed to resolve the transaction:
l/jp-desktop-nvidia:latest => #25 1.952 No match for argument:
l/jp-desktop-nvidia:latest => #25 1.952 No match for argument:
l/jp-desktop-nvidia:latest => #25 1.952 No match for argument:
l/jp-desktop-nvidia:latest => #25 1.952 Package "toolbox-0.1.1-1.fc41.x86_64" is already installed.
l/jp-desktop-nvidia:latest => #25 1.952 Package "udisks2-2.10.1-6.fc41.x86_64" is already installed.
l/jp-desktop-nvidia:latest => #25 1.952 You can try to add to command line:
l/jp-desktop-nvidia:latest => #25 1.952   --skip-unavailable to skip unavailable packages
l/jp-desktop-nvidia:latest => #25 1.963 ============================= Failed 'dnf' Module =============================

This indicates to me that it's seeing the empty args as packages. This might be a case where we actually don't want to encapsulate the args in ".

modules/dnf/dnf.sh Outdated Show resolved Hide resolved
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
modules/dnf/dnf.sh Outdated Show resolved Hide resolved
@gmpinder
Copy link
Member

I'm getting an error when running a basic install:

l/jp-desktop-nvidia:latest => #25 0.447 ============================== Start 'dnf' Module ==============================
l/jp-desktop-nvidia:latest => #25 0.531 Installing RPMs
l/jp-desktop-nvidia:latest => #25 0.531 Installing: @kde-desktop
l/jp-desktop-nvidia:latest => #25 0.578 Updating and loading repositories:
l/jp-desktop-nvidia:latest => #25 0.800  Fedora 41 openh264 (From Cisco) - x86_ 100% |   6.5 KiB/s | 989.0   B |  00m00s
l/jp-desktop-nvidia:latest => #25 1.161  Fedora 41 - x86_64                     100% |  85.6 KiB/s |  30.8 KiB |  00m00s
l/jp-desktop-nvidia:latest => #25 1.256  Fedora 41 - x86_64 - Updates Archive   100% |  36.2 KiB/s |   3.4 KiB |  00m00s
l/jp-desktop-nvidia:latest => #25 1.416  Fedora 41 - x86_64 - Updates           100% | 184.9 KiB/s |  29.4 KiB |  00m00s
l/jp-desktop-nvidia:latest => #25 1.483 Repositories loaded.
l/jp-desktop-nvidia:latest => #25 1.952 Failed to resolve the transaction:
l/jp-desktop-nvidia:latest => #25 1.952 No match for argument:
l/jp-desktop-nvidia:latest => #25 1.952 No match for argument:
l/jp-desktop-nvidia:latest => #25 1.952 No match for argument:
l/jp-desktop-nvidia:latest => #25 1.952 Package "toolbox-0.1.1-1.fc41.x86_64" is already installed.
l/jp-desktop-nvidia:latest => #25 1.952 Package "udisks2-2.10.1-6.fc41.x86_64" is already installed.
l/jp-desktop-nvidia:latest => #25 1.952 You can try to add to command line:
l/jp-desktop-nvidia:latest => #25 1.952   --skip-unavailable to skip unavailable packages
l/jp-desktop-nvidia:latest => #25 1.963 ============================= Failed 'dnf' Module =============================

This indicates to me that it's seeing the empty args as packages. This might be a case where we actually don't want to encapsulate the args in ".

Ok just tested my suggested changes locally. They do fix the problem I ran into.

fiftydinar and others added 8 commits December 24, 2024 12:59
… part 1

Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
… part 2

Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
… part 3

Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
… part 4

Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
… part 5

Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
… part 6

Co-authored-by: Gerald Pinder <gmpinder@gmail.com>
echo "Removing: ${GROUP_REMOVE[*]}"
echo "Installing: ${GROUP_INSTALL[*]}"
dnf -y group remove "${GROUP_REMOVE[@]}"
dnf -y ${WEAK_DEPS_FLAG} group install --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} "${GROUP_INSTALL[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Removing: ${GROUP_REMOVE[*]}"
echo "Installing: ${GROUP_INSTALL[*]}"
dnf -y group remove "${GROUP_REMOVE[@]}"
dnf -y ${WEAK_DEPS_FLAG} group install --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} "${GROUP_INSTALL[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Removing: ${GROUP_REMOVE[*]}"
echo "Installing: ${GROUP_INSTALL[*]}"
dnf -y group remove "${GROUP_REMOVE[@]}"
dnf -y ${WEAK_DEPS_FLAG} group install --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} "${GROUP_INSTALL[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Removing: ${GROUP_REMOVE[*]}"
echo "Installing: ${GROUP_INSTALL[*]}"
dnf -y group remove "${GROUP_REMOVE[@]}"
dnf -y ${WEAK_DEPS_FLAG} group install --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} "${GROUP_INSTALL[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

elif [[ ${#GROUP_INSTALL[@]} -gt 0 ]]; then
echo "Installing RPM groups"
echo "Installing: ${GROUP_INSTALL[*]}"
dnf -y ${WEAK_DEPS_FLAG} group install --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} "${GROUP_INSTALL[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

elif [[ ${#REMOVE_PKGS[@]} -gt 0 ]]; then
echo "Removing RPMs"
echo "Removing: ${REMOVE_PKGS[*]}"
dnf -y remove ${REMOVE_UNUSED_DEPS_FLAG} "${REMOVE_PKGS[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Replacing packages from repository: '${REPO}'"
echo "Replacing: ${PACKAGES[*]}"

dnf -y ${WEAK_DEPS_FLAG} distro-sync --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} --repo "${REPO}" "${PACKAGES[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Replacing packages from repository: '${REPO}'"
echo "Replacing: ${PACKAGES[*]}"

dnf -y ${WEAK_DEPS_FLAG} distro-sync --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} --repo "${REPO}" "${PACKAGES[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Replacing packages from repository: '${REPO}'"
echo "Replacing: ${PACKAGES[*]}"

dnf -y ${WEAK_DEPS_FLAG} distro-sync --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} --repo "${REPO}" "${PACKAGES[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

echo "Replacing packages from repository: '${REPO}'"
echo "Replacing: ${PACKAGES[*]}"

dnf -y ${WEAK_DEPS_FLAG} distro-sync --refresh ${SKIP_UNAVAILABLE_FLAG} ${SKIP_BROKEN_FLAG} ${ALLOW_ERASING_FLAG} --repo "${REPO}" "${PACKAGES[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants