bugGNU Octave - Bugs: bug #57122, pkg uninstall incorrectly errors...

 
 

bug #57122: pkg uninstall incorrectly errors out on unrelated dependencies

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Sun 27 Oct 2019 03:34:35 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  3 - Low Item Group:  Incorrect Result
Status:  Fixed Assigned to:  philipnienhuis
Originator Name:  Philip Nienhuis Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 31 Oct 2019 05:57:47 PM UTC, comment #10: 

2Rik: thank you for the polishing :-)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 31 Oct 2019 04:09:11 PM UTC, comment #9: 

I renamed "in_or_un" to "uninstall_flag" and "tbdeleted_packages" to "to_delete_packages".  I checked it in here https://hg.savannah.gnu.org/hgweb/octave/rev/352cead49726.

Thanks Philip for the fix.

Rik <rik5>
Group administrator
Thu 31 Oct 2019 12:20:19 PM UTC, comment #8: 

Thx for reviewing.

tbdeleted_packages = "list" of packages to be deleted, analogous for remaining_packages = list of packages to be kept installed.

"in_or_un" is a logical variable standing for *in*stalling or *un*installing a package.
Inside get_unsatisfied_deps.m the (input) variable "installed_pkgs_lst" could be renamed to just "packages_list" (5 occurrences in total IIRC) as depending on the value of in_or_un the list comprises packages to be kept or to be deleted.

If you can come up with better mnemonic variable names and a better commit message, good, I'm no native English speaker :-)

BTW I tested installing & uninstalling with and without dependencies and inverse dependencies on Linux and Windows, I saw no regressions.
Too bad that it's apparently hard to come up with BIST tests for pkg.m

Philip Nienhuis <philipnienhuis>
Group Member
Wed 30 Oct 2019 11:55:12 PM UTC, comment #7: 

I verified the patch works fine with the example given in comment #2.

What does the variable named "in_or_un" stand for?  Is there a clearer name?

What does the variable named "tbdeleted_packages" stand for?  Is there a clearer name?

Rik <rik5>
Group administrator
Wed 30 Oct 2019 10:25:43 PM UTC, comment #6: 

The proposed fix turned out to be remarkably simple, see attached file.


(file #47770)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 28 Oct 2019 06:50:03 PM UTC, comment #5: 

I'm happy to work on this.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 28 Oct 2019 06:09:42 PM UTC, comment #4: 

Thanks for bringing this up. I've been adding "-nodeps" to pkg commands for so long I forgot about this.

When uninstalling a package, it's probably sufficient to check the list of installed packages for reverse dependencies on that package.

I agree with Rik, no need to warn about missing dependencies from completely unrelated packages.

Mike Miller <mtmiller>
Group Member
Mon 28 Oct 2019 12:18:40 AM UTC, comment #3: 

Confirmed.  I agree that for dependencies that are unrelated to the package that is being uninstalled there is no reason to error.  In fact, it's questionable whether we should even warn about it since it is an unrelated matter.  In any case, I think your proposed strategy for a fix looks good.

Rik <rik5>
Group administrator
Sun 27 Oct 2019 08:26:11 PM UTC, comment #2: 

Easy enough.

Install just signal with -nodeps flag (as it needs control).
Next, install e.g., ga (has no dependencies and signal doesn't depend on it).
Then:

pkg uninstall ga

will lead to the error, complaining signal needs control. Of course neither signal nor control have any relationship with ga.
This scenario may be a little far fetched but if a user has knowingly installed a package w/o all req'd dependencies, he/she shouldn't be stopped from uninstalling some unrelated package. IMO it violates the principle of least surprise.

Of course the workaround is to specify "-nodeps"; but the way the error is generated is just flawed.


I've already investigated and have some idea of how to fix it.

The current logic is to single out the package(s)-to-be-uninstalled and then cycle through all the rest and see if any of those remaining package's dependencies will remain in the set of remaining packages. A bit convoluted, but it'll work. But: the flaw is that dependencies that were already missing before the current "pkg uninstall" action are inevitably detected as well and will lead to an error.

It would be much simpler, more direct and easier to understand to cycle through the remaining packages but rather check if any of their dependencies is in the list of packages-to-be-uninstalled.

Admittedly, having looked at the pkg.m code in some detail I think at places it is very cleverly written but there are ample potential flaws, often based on undue expectations.
This bug is a fine example: pkg.m expects a consistent package database w/o lacking dependencies, but users may have their reason to not fulfill all package dependencies.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 27 Oct 2019 06:57:08 PM UTC, comment #1: 

@Phillip: Do you have a sequence of actions that leads to this error so that others can reproduce?

Rik <rik5>
Group administrator
Sun 27 Oct 2019 03:34:35 PM UTC, original submission:  

(Sorry, title could be better, suggestions?)

When trying to uninstall a package that no other installed package depends on while there are missing dependencies elsewhere, pkg.m emits an error message and stops:

>> pkg uninstall octproj
error: the following dependencies where unsatisfied:
   geometry needs matgeom >= 1.0.0


Indeed geometry-4.0.0 needs matgeom (that was indeed missing), but none of them depends on octproj. IMO octproj could safely be uninstalled.

Expected result (in this case):
The error message had better be a warning, and octproj should be uninstalled.

The relevant code isn't easy to follow, but if confirmed I'll try to come up with a cset.

Philip Nienhuis <philipnienhuis>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-10-31 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2019-10-30 rik5 StatusPatch Submitted Patch Reviewed
    2019-10-30 philipnienhuis Attached File- Added bug_57122_uninstall_w_unrelated_deps.cset, #47770
        StatusConfirmed Patch Submitted
    2019-10-28 philipnienhuis Assigned toNone philipnienhuis
    2019-10-28 rik5 StatusNeed Info Confirmed
    2019-10-27 rik5 Priority5 - Normal 3 - Low
        StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code