archlinux-downgrade / downgrade

Downgrade packages in Arch Linux
GNU General Public License v2.0
570 stars 24 forks source link

Create `pacignore` script #196

Closed atreyasha closed 2 years ago

atreyasha commented 2 years ago

Checklist

Description

This PR aims to integrate pacignore into downgrade as discussed previously. So far I did the following:

  1. Migrate downgrade and pacignore scripts into dedicated bin directory
  2. Update Makefile to install both scripts given new locations
  3. Update unit tests with new paths and specifications for pacignore as discussed previously

@pbrisbin WDYT about the diffs so far?

Next steps

Besides the usual housekeeping tasks in the checklist above, I want to add the following features in this PR:

~1. Add an includes subcommand to pacignore to check if a package is currently being ignored, for example pacignore includes foo to check if foo is currently being ignored. This feature would simplify the prompt_to_ignore function in the downgrade script and would reduce some boilerplate code in the pacignore script.~

  1. Add pacignore invocations inside the prompt_to_ignore function in the downgrade script
  2. Update GitHub workflows and restyled to also check pacignore

Regarding implementing point 1 above, I plan to change the prompt_to_ignore function to the following:

prompt_to_ignore() {
  local pkg ans

  for pkg; do
    pacignore check --pacman-conf "$PACMAN_CONF" "$pkg" && return 0
    eval_gettext "add \$pkg to IgnorePkg? [y/N] "
    read -r ans
    if [[ "${ans,,}" == $(gettext 'y')* ]]; then
      pacignore add --pacman-conf "$PACMAN_CONF" "$pkg"
    fi
  done
}

I tested this locally and it works fine with a system-level install. However, I am not sure how to make this work successfully for the unit tests. This is because the current unit tests source functions from downgrade and pacignore and are not able to execute scripts per-se (such as pacignore in the example above) because we export LIB=1 in helper*.sh.

Does it make sense to mask pacignore with a no-op for the test/prompt_to_ignore unit tests, since we already have dedicated unit tests for pacignore in test/pacignore? That way, we would only be testing how prompt_to_ignore behaves with "y" or "n" answers and would keep everything related to pacignore separate.

atreyasha commented 2 years ago

Apologies for the late response.

One high-level thought I had is that we're extracting a new (smaller, simpler) executable here, but we're still doing a lot of the "noisy" things we do in downgrade that were specifically necessary because of its higher level of complexity. WDYT?

That's true, I initially just modeled the syntax and flow in pacignore from that of downgrade and did not put too much thought into simplicity. But I agree, ultimately we are working towards making things "simpler" and what you mentioned is indeed a step towards that.

Let me know your thoughts on the changes made in this direction