From dc4b361426d9e237dbb71057ca3799a8223f7eac Mon Sep 17 00:00:00 2001 From: John McQuah Date: Thu, 14 Sep 2023 17:56:52 -0400 Subject: [PATCH] use a common style to show missing or already-installed ports (FS#1843) be more selective in applying the forceRebuild flag --- doc/FS1843.md | 72 ++++++++++++++++++++++++++++++++++++++ src/installtransaction.cpp | 10 ++++-- src/prtget.cpp | 45 +++++++++++++----------- 3 files changed, 105 insertions(+), 22 deletions(-) create mode 100644 doc/FS1843.md diff --git a/doc/FS1843.md b/doc/FS1843.md new file mode 100644 index 0000000..c529410 --- /dev/null +++ b/doc/FS1843.md @@ -0,0 +1,72 @@ +# Flyspray 1843 notes +## Was: Re: Please make the output of prt-get --test more common + +teodor observed that prt-get --test gives two different styles of output. +The output style was found to be affected by the number of packages requested +on the command line (one versus many), and by whether dependency resolution +was toggled (install versus depinst). + +In the case when one package was requested for installation, an early check +of /var/lib/pkg/db for the existence of a previous installation would alert the +user to the malformed command. No such check of /var/lib/pkg/db was +being performed for multiple packages passed on the command line (FS#1910), and +so the program continued all the way through executeTransaction() and +evaluateResult(), which produced the more verbose output that teodor noted. + +A similar discrepancy could be observed when requesting an **update** of **one** +package that was not previously installed, versus an update of **multiple** +packages among which there are some not yet installed. You would have seen a +terse warning in the first case, and a more verbose output in the second case. + +The softdeps branch in this code base is now more consistent in performing the +scan of /var/lib/pkg/db, no matter how many packages are passed as argument. +But the mixed-upinst branch collapses the distinction between install and +update, always bringing up to date all the ports passed as argument (and their +dependencies, unless --nodeps is given), so it can postpone the scan of +/var/lib/pkg/db until the step of customizing pkgadd flags for each target. +Ports exhibiting no version differences between the repos and /var/lib/pkg/db +would be left alone, unless forceRebuild was requested. + +teodor's second observation is more interesting. During dependency resolution, +the invalid arguments (ports not found in the active repos) were silently being +dropped, rather than being kept in memory for a post-transaction report. If +**all** ports passed to a ``depinst`` command are nowhere among the active +repositories, then the empty set is what gets used for executeTransaction(), +with the predictable result "no package specified for install". Why did we see +a different result when running ``install``? Because the calcDependencies() +method was never called, and so executeTransaction() received the entire list +of nowhere-to-be-found ports! Iterating through this list of nonexistent ports +would populate the missingPackages list, which would then be displayed nicely +during the post-transaction summary. + +Although we do populate a missingPackages list during calcDependencies(), the +contents of this list were not being included in the post-transaction +summary. As the code was originally conceived, executeTransaction() would get +a filtered list, with missing packages omitted. This side-effect of ``depinst`` +violates the principle of least surprise, because users like teodor might +justifiably expect toggling dependency resolution to generate a superset of +**their argument list** $@, not just a superset of the valid ports in $@. To be +more faithful to the principle of least surprise, the softdeps branch now +propagates into executeTransaction() even the ports that are not in the repos, +so that the post-transaction summary can display them. + +As for teodor's request to unify into one list the post-transaction summary +of missing ports and already-up-to-date ports, I think the possible benefits +for easier parsing are overstated. Some users might not want the two lists +mixed together, for instance when the dependency tree of a valid target ends +up drowning out all the ports that are not in the repos. Two separate lists +would offer greater readability in most cases. + +teodor's proposed heading for a united list, "The following ports were +not found/already installed:", suggests a new way for the softdeps branch to +handle FS#1910. All arguments to ``install`` and ``depinst`` will be tested +against /var/lib/pkg/db to ensure that they're not already installed, and if +all the tests fail then the user is immediately alerted to the malformed +command. This preliminary check is simply jw's original design, but extended +from one port to many. Then, if any of the requested ports is not yet +installed, we can go ahead with initRepo() and attempt a less ambitious +transaction (limited to a subset of the user's argument list). The program +will reach evaluateTransaction() and display what succeeded, as well as what +was wrong with the original command. This approach will give users at +least partial results from a malformed command, while also providing an +informative message so that a revised command can be issued later. diff --git a/src/installtransaction.cpp b/src/installtransaction.cpp index a0244cf..3e6668c 100644 --- a/src/installtransaction.cpp +++ b/src/installtransaction.cpp @@ -252,7 +252,7 @@ InstallTransaction::installPackage( const Package* package, commandName = "prt-cache"; } - // - initial information about the package to be build + // - initial information about the package to be built string message; message = commandName + ": "; if (update) { @@ -341,6 +341,12 @@ InstallTransaction::installPackage( const Package* package, } string args = "-d " + parser->pkgmkArgs(); + // do not force a rebuild unless the port was given on the command line + if ( parser->pkgmkArgs().find(" -f")==string::npos && + find(parser->otherArgs().begin(), parser->otherArgs().end(), + package->name()) == parser->otherArgs().end() ) { + StringHelper::replaceAll(args," -f", ""); + } Process makeProc( cmd, args, fdlog ); if ( makeProc.executeShell() ) { result = PKGMK_FAILURE; @@ -667,7 +673,7 @@ InstallTransaction::calcDependencies( ) } } if ( !validPackages ) { - return PACKAGE_NOT_FOUND; + return NO_PACKAGE_GIVEN; } if ( !calculateDependencies() ) { diff --git a/src/prtget.cpp b/src/prtget.cpp index 7806635..a0643c8 100644 --- a/src/prtget.cpp +++ b/src/prtget.cpp @@ -614,23 +614,29 @@ void PrtGet::install( bool update, bool dependencies ) } } else { for ( ; it != args.end(); ++it ) { - string s = *it; + string s = *it; if ( m_pkgDB->isInstalled( s ) ) { - // pkgadd will fail on these, since it won't be given the -u flag - invalidArgs.push_back( s ); + // pkgadd will fail on these, since it won't be given the -u flag + invalidArgs.push_back( s ); } - } + } } - if ( invalidArgs.size() > 0 ) { - string attemptedOp = ( update ) ? "update" : "install"; - attemptedOp += " the following packages "; - attemptedOp += ( update ) ? "(not yet installed)" : "(already installed)"; - cout << "cannot "<< attemptedOp < 0 && update) || + (invalidArgs.size() == args.size() && !update) ) { + string invalidHeader = "-- Packages installed before this run (ignored)"; + if (update) { invalidHeader = "-- Cannot update the following (not yet installed)"; } + if (m_parser->isTest()) { + cout << "*** prt-get: test mode" << endl << endl; + } + cout << invalidHeader << endl; list::const_iterator it = invalidArgs.begin(); for ( ; it != invalidArgs.end(); ++it ) { cout << *it << endl; } + if (m_parser->isTest()) { + cout << "*** prt-get: test mode end" << endl; + } m_returnValue = PG_GENERAL_ERROR ; return; } @@ -649,18 +655,17 @@ void PrtGet::install( bool update, bool dependencies ) cerr << "prt-get: cyclic dependencies found" << endl; m_returnValue = PG_GENERAL_ERROR; return; - } else if ( result == InstallTransaction::PACKAGE_NOT_FOUND ) { - warnPackageNotFound(depTransaction); - m_returnValue = PG_GENERAL_ERROR; - return; } - const list& depRef = depTransaction.dependencies(); - list::const_iterator it = depRef.begin(); + list deps = depTransaction.dependencies(); + const list< pair >& depMissing = depTransaction.missing(); + list< pair >::const_iterator it = depMissing.begin(); - list deps; - for (; it != depRef.end(); ++it) { - if (!m_pkgDB->isInstalled(*it)) { - deps.push_back(*it); + // ensure that missing ports are retained for the post-transaction + // summary (FS#1843). Exempt any ports that were installed manually + // or from a repo that has since been deactivated. + for (; it != depMissing.end(); ++it) { + if (! m_pkgDB->isInstalled(it->first)) { + deps.push_back(it->first); } } @@ -1177,7 +1182,7 @@ void PrtGet::evaluateResult( InstallTransaction& transaction, cout << endl; - if ( errors == 0 && !interrupted ) { + if ( inst.size() && errors == 0 && !interrupted ) { cout << "prt-get: " << command[1] << " successfully" << endl; } else { m_returnValue = PG_PARTIAL_INSTALL_ERROR;