Spike separate utilities

[pbrisbin]

This takes a stab at breaking downgrade into three separate utilities:

• downgrade-search

  Hunt for packages to downgrade to in the usual way (cache, ALA, operaters, etc) and output them in a formatted, sorted list.

• downgrade-select

  Present that list to the user for selection and output the line that was selected.

  As you can see, this is just exec fzf to match current behavior. If we wanted to implement our own "native" dowgrade-select in order to make a picker an optdepend, there would be quite a bit of complexity here.

• dowgrade-install

  "Parse" the selected line, install the package, and handle auto-IgnorePkg

This is just a Spike to help discussion by giving a concrete example to discuss.

My thoughts so far:

I really like the separation of concerns. I can see how we could refactor our test suite to be per-script and end up with really a really comprehensive suite with minimal "hacks".

It won't function without solving one big problem: reading user input while processing stdin for data. Right now, if you run this, the install step just shows the [Y/n] prompt and dies. Presumably this is because you can't get the user interaction while stdin is tied up reading the output of downgrade-search. There's clearly a solution here, as fzf is doing just that, but I'm not sure how terrible it'll look in pure Bash -- maybe it'll be fine, but that's probably the next thing to explore.

I kind of like including the path-or-URL as the final item in the list sent to fzf. This simplifies away the need to maintain the "candidates" list and pulling an entry out by number. We simply echo the selected line and know that the last column is the thing to pacman -U. Simple. We could probably drop the number of the list, which could simplify present_packages even more.

I noticed a few bugs along the way that might be worth reporting:

• We still sometimes parse pacman.conf ourselves; we should use pacman-conf uniformly
• When we do use pacman-conf, we don't respect our own --pacman-conf option; we should be passing that along as --config for uniform behavior
• We should use Bash's getopt, IMO, to normalize options so we don't have to do so much of our own validation
• I think we could ditch read_unique; did we make that in response to a real Issue/request? It's minor complexity but also seems kind of unnecessary to me in retrospect.

[pbrisbin]

Ping @atreyasha

[atreyasha]

First off, thanks for making this initial PR. I think its super useful to start our discussion :nerd_face:

Thoughts

I can see how we could refactor our test suite to be per-script and end up with really a really comprehensive suite with minimal "hacks".

Yes indeed. And perhaps it would be good if we keep the same function names with the LIB environmental variable in each script. That way we could easily source the functions in all scripts and test them similarly.

It won't function without solving one big problem: reading user input while processing stdin for data.

Ah I understand now. In this case the pipe makes things slightly complicated, but I have some ideas on how to work around it. Will try something in the coming days.

We should use Bash's getopt, IMO, to normalize options so we don't have to do so much of our own validation

True, this would be helpful. But IMO this PR has a lot at the moment. I think it would be helpful to focus on this translation in a separate PR. See question 1 below.

I kind of like including the path-or-URL as the final item in the list sent to fzf.

Oh nice touch! Yes, I think this is a good step going forward!

We still sometimes parse pacman.conf ourselves; we should use pacman-conf uniformly. When we do use pacman-conf, we don't respect our own --pacman-conf option; we should be passing that along as --config for uniform behavior

Ah nice, this would directly address #171.

I think we could ditch read_unique; did we make that in response to a real Issue/request?

No real issue, that was just me being finicky :D Agree to remove it.

Questions

1. WDYT about opening another issue and handling a getopt translation in a separate PR down the line? If you agree, I will open the issue.

2. At the moment for me, sudo ./bin/downgrade <pkg> exits with an error message error: no targets specified (use -h for help). This has to do with CLI options "${search_options[@]}" being passed to downgrade-search with the latter being unable to parse these options: https://github.com/pbrisbin/downgrade/blob/ea2fd934d23be49c97e7dd45c2bacd2ef27d3b0d/bin/downgrade#L129

   1. An easy fix I could see here is to communicate with downgrade-search, downgrade-select and downgrade-install only via environmental variables. That would also mean we do all the CLI parsing and variable expansion in downgrade and then we pass those variables to the other scripts. But this would create a discrepancy between the scripts, where only downgrade handles CLI options and parsing while the others expect environmental variables.
   2. A "harder" approach would be to allow parsing in each script. But this might be better since it would allow each script to be independent and operate consistently with CLI options. If we decide this option, maybe it would make sense to make a separate parser script that is sourced by each of our downgrade* scripts.
   3. WDYT about these options?

3. Going forward, I think it would make sense for us to split some tasks in this PR. Do you have any preferences for tasks? I am flexible, so I will just take up whichever tasks are leftover.

[pbrisbin]

perhaps it would be good if we keep the same function names with the LIB environmental variable in each script. That way we could easily source the functions in all scripts and test them similarly.

I'm OK with that but I wouldn't prefer it ideally. It couples the tests to how we organize the functions, which is a layer too low (IMO) for "integration" tests. It also allows for sneaky side-channel behavior influencing. That cuts both ways: sometimes you need it to test, but sometimes taking that option away from yourself results in better tests.

I think with the separate scripts we should be able to invoke them from the outside to get the desired coverage. In a way, we're calling those three scripts and their usage our "API" and testing at that API (not below it) it ideally (again, IMO), because it'll give us design feedback as well as totally verify the controls users interact with too.

But IMO this PR has a lot at the moment. I think it would be helpful to focus on this translation in a separate PR

opening another issue and handling a getopt translation in a separate PR down the line?

Agree to make it a separate issue, but I'm not committing to the order of tackling it just yet. Once we are sure this direction works, we can lay out a roadmap of changes to do in isolation to get there. Tackling getopt may make sense to do first, or defer for later.

At the moment for me, sudo ./bin/downgrade <pkg> exits with an error message

Ah sorry, that was a late change to show how options passing might work. I didn't realize it would break the hard-coding of options in called script by passing them -- I had thought they'd be ignored.

A "harder" approach would be to allow parsing in each script. But this might be better since it would allow each script to be independent and operate consistently with CLI options.

Yes, I'd prefer this. If we're using getopt, I'm optimistic the parsing can be low enough burden that it's OK to "do it twice" in a way.

If we decide this option, maybe it would make sense to make a separate parser script that is sourced

I would rather not do this. We might end up there if enough reasons pile up, but introducing a centralized "lib" that is sourced by the distinct CLIs re-introduces coupling, which I'm trying to get away from with this design. Again, we might end up there if more trade-offs become apparent, but I'd like to start by pushing back against it.

Do you have any preferences for tasks?

I don't have a preference, per se, but I can give it some thought and try to outline an order of operations that I think might make sense.

To confirm: you're on board with this general re-design overall?

[atreyasha]

In a way, we're calling those three scripts and their usage our "API" and testing at that API (not below it) it ideally (again, IMO), because it'll give us design feedback as well as totally verify the controls users interact with too.

Ah I understand now. Agree to modify towards script-level unit testing.

I would rather not do this. We might end up there if enough reasons pile up, but introducing a centralized "lib" that is sourced by the distinct CLIs re-introduces coupling

Understood. Agree to start off by having scripts fully decoupled.

I can give it some thought and try to outline an order of operations that I think might make sense.

Yes that would be great

To confirm: you're on board with this general re-design overall?

On board :+1:

[pbrisbin]

So here's how I would approach it -- these don't necessarily have to be a single Issue/PR per, but they could be.

Pre-requisite / clean-up:

1. Fix the bugs around --pacman-conf
2. Remove read_unique
3. Migrate to getopt(1)
4. Remove our #candidates == 0, == 1 branches for those fzf options

5. Update the output lines to include path-or-url as the last element

   Perhaps remove the numbering in the output, but definitely stop holding onto candidates and looking up by number.

First big push, expose and answer any unknowns about the idea:

1. Extract downgrade-install

   Feed the output of fzf directly to it and parse the name, path-or-url -- like I have in this PR. Replace any tests related to this to be something like $ echo "something" | downgrade-install <options>.

   Here, we'll need to solve the "how to read user-input while processing stdin" problem. I'm not sure how robust this is, but I was able to prove that you can simply read /dev/tty instead of stdin:

   [patrick@prince platform]$ tester() {
   > x=$(cat) # stdin
   > echo 'prompt'
   > read -r ans </dev/tty # user input
   > echo "stdin was $x"
   > echo "answer was $ans"
   > }
   [patrick@prince platform]$ echo hi | tester
   prompt
   yes
   stdin was hi
   answer was yes

Then, keep on truckin:

1. Extract downgrade-select (just exec fzf), probably remove any tests about this since we're doing nothing
2. Extract downgrade-search, rewrite tests as simply: $ downgrade-search <options>
3. Implement our own downgrade-select for when fzf is not found, add logic to downgrade to support trying a few, an option for the user to specify, and the fallback, etc. Probably need lots of tests again now :(
4. Finally, make fzf an optdepend :tada:

[atreyasha]

Sounds like a solid plan! Now that we have several open bug reports, I would prefer to start off by addressing them. Once I am done, will see where I can help out in this plan.

IMO it also makes sense to make a larger roadmap for downgrade in terms of planned developments for the next major release. I was thinking of doing this in the form of a pinned issue called v12.0.0 roadmap and then describing the major issues/developments that we aim to address till then.

I think it would help me at least with prioritizing, given that downgrade seems to be slowly growing into a larger and more complex project. WDYT?

[pbrisbin]

WDYT?

Sure, you do you :)

[pbrisbin]

Closing this now that it's done its job. :tada: