CRUX

Welcome to CRUX bug tracking.
Tasklist

FS#1910 - `prt-get update foo bar` does not check whether foo or bar is installed

Attached to Project: CRUX
Opened by John McQuah (farkuhar) - Monday, 23 May 2022, 19:58 GMT
Task Type Bug Report
Category tools → prt-get
Status New
Assigned To No-one
Operating System CRUX
Severity Medium
Priority Normal
Reported Version 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

(credit to beerman for discovering this behaviour) When prt-get install (or update) is given an argument list with more than one port, it skips the preliminary check of whether those ports are installed. This useful safeguard is only exercised when exactly one port is given on the command line.

I wrote a patch that reinterprets such a malformed command in the most charitable way: any ports for which install (or update) makes sense are retained; the rest are discarded with a warning. If none of the ports on the command line are compatible with the requested action, then the command fails in the same manner that `prt-get update foo` would fail, when foo is not installed.

TODO: the output would be even nicer if the warnings about ignored (incompatible) arguments could be postponed until the post-transaction summary, rather than getting lost in the scrollback buffer as the usable arguments get processed by pkgmk and pkgadd. But prt-get doesn't provide a straightforward public method for manipulating the contents of m_ignore (populated by argparser.cpp), which would be the most natural place to preserve the information for a post-transaction summary.
   upinst-clean-input.patch (2.6 KiB)
This task depends upon

Comment by Johannes Winkelmann (jw) - Monday, 23 May 2022, 20:58 GMT
I'm too far away to have any string feelings on the patch itself, although I feel it may be more intuitive to simply fail with a warning if you provide a list of ports to update and one isn't previously installed, rather than doing part of the work and failing at the rest.

That said, m_ignore is a direct mapping of a command line argument that lists port the _user_ wants to ignore, and as such should _not_ be used for ports the user explicitly wanted to update but failed at the pkgadd stage. An argument parser is not a natural place to store information about a transaction's success (or lack thereof)
Comment by John McQuah (farkuhar) - Monday, 23 May 2022, 22:09 GMT
I think you and jaeger are right: providing an informative error message and aborting immediately (so that a properly formed command can be issued) is a more intuitive response than carrying out a subset of the tasks requested. You could easily get that behaviour by changing the condition ( compatible_args.size() < 1 ) to ( compatible_args.size() < args.size() ) in my patch.

As for my description of m_ignore as a "natural place" for preserving information, I admit that wasn't the best phrasing. Among the few objects that are accessed during post-transaction reporting, m_ignore stood out as a prime candidate because it gets used in the method that prints the result of an InstallTransaction. Since m_ignore is not meant to be manipulated outside of the argument parser, another object would have to be introduced (and referenced by the evaluateResult method), if we wanted prt-get to have the behaviour of my original patch and still give an informative warning about unused arguments in the post-transaction report.

At the end of the TODO file there's the question "should there be a install/update mixed to mode, to install and update, which ever method is needed?" Implementing this idea would require a heavier refactoring of prtget.cpp than the simple subsetting that my patch performs, but it's another way to address the bug that beerman found.
Comment by John McQuah (farkuhar) - Friday, 24 March 2023, 18:44 GMT
So I finally got around to implementing the TODO suggestion of a mixed install/update mode, which made it easy to also implement the suggestion "allow injecting of new (uninstalled) dependencies." In this mode of operation, there's no need to check the installation status of anything given on the command line, which means "prt-get depinst" behaves more like "apt-get install" (at least if I'm reading the apt-get manpage correctly -- I don't currently have a Debian installation to confirm). See the resulting fork here: https://git.crux.nu:82/farkuhar/prt-get/src/branch/mixed-upinst

The most recent commit to that branch, "command consolidation", refers to the elimination of some duplicate code, and slight tweaks to the argument parser so that commands like "dependent" and "sysup" could be implemented by calling a closely-related function whose behaviour is now influenced by some global boolean variables (also set during argument parsing). The inline comments in the original code were practically begging for this kind of refactoring.

There were a few command-line flags (--nodeps and --depsort) which were not getting used as extensively as they could have been, so I gave them additional roles in controlling the behaviour of install/update/sysup. In the "mixed-upinst" branch the default behaviour is to resolve dependencies for all three commands, but by passing the flag --nodeps you get the old behaviour of install/update.

There's also a new flag --softdeps (inspired by a recent beerman comment on sysup behaviour), so that the sorting algorithm can take optional dependencies into account. Requests for this feature date back to the introduction of the "Optional:" metadata field in 2008 (https://lists.crux.nu/pipermail/crux-devel/2008-May/003375.html), but in that thread they encountered resistance from jue and tilman (who claimed that if our official tools added support for such a feature, then maintainers would feel obliged to use it). This objection is undermined by the fact that the metadata field I shoved aside to implement sorting by soft dependencies, "Packager:", was officially supported by "prt-get info" and "prt-get printf" but still wasn't filled out by the majority of port maintainers.

Anyway, I hope there's some interest from other CRUXers to try out this fork of prt-get.

Loading...