Bug 325636 - Opening a markings file might give incorrect warning
Summary: Opening a markings file might give incorrect warning
Status: RESOLVED WORKSFORME
Alias: None
Product: libqapt
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Harald Sitter
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-04 17:04 UTC by Vassilis Palassopoulos
Modified: 2021-03-10 10:11 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
A relatively long markings file (1.08 KB, text/plain)
2013-10-04 17:07 UTC, Vassilis Palassopoulos
Details
A smaller version of the markings file (bug doesn't appear) (385 bytes, text/plain)
2013-10-04 17:11 UTC, Vassilis Palassopoulos
Details
A smaller version of the markings file (bug appears) (504 bytes, text/plain)
2013-10-04 17:12 UTC, Vassilis Palassopoulos
Details
Don't interrupt method when pkgIter.end() becomes true (1.62 KB, patch)
2013-10-14 09:08 UTC, Vassilis Palassopoulos
Details
A markings file where not all markings get processed (674 bytes, text/plain)
2013-10-16 08:49 UTC, Vassilis Palassopoulos
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vassilis Palassopoulos 2013-10-04 17:04:24 UTC
Muon has the ability to open a markings file (where package changes are declared in a specific manner), process it and mark any changes to be applied when the user confirms them unless the markings file isn't processable (e.g. the packages weren't declared in a way that Muon understands) in which case it will show an appropriate error message to the user.

Sometimes it shows the error message even though it can process the markings file, leaving the user with the impression that the processing was unsuccessful.

Reproducible: Sometimes

Steps to Reproduce:
1. Open Muon Package Manager
2. Go to File ---> Read Markings...
3. Select a Markings file
Actual Results:  
It incorrectly shows a window with the message: "Could not mark changes. Please make sure that the file is a markings file created by either the Muon Package Manager or the Synaptic Package Manager"

The markings file gets processed successfully since the packages are marked (as can be seen on the packages list). The "Preview changes", "Apply Changes", "Undo", "Unmark All" buttons are greyed out and the status bar doesn't update its status.

If its status gets re-updated (e.g. marking a package) then the buttons appear normally and the status bar shows the updated statistics. The "Preview changes" shows correctly all the marked changes and the "Apply Changes" applies the changes correctly.

Expected Results:  
Process the markings file and show the markings in the GUI (by updating the previously greyed out buttons and updating the status bar) without showing incorrectly the error message leaving the impression that the processing was unsuccessful.

The bug appears on Kubuntu 13.10 64-bit beta 2 with Muon 2.0.65.

It seems that the bug is within the libqapt component, specifically on backend.cpp method: bool Backend::loadSelections(const QString &path)

Possibly in some cases the method returns the wrong boolean value ('false' instead of 'true'). Probably when Muon receives the 'false' boolean value, it doesn't try to update it's interface as it thinks that the processing was unsuccessful (though on the packages list, the packages are marked with the changes).
Comment 1 Vassilis Palassopoulos 2013-10-04 17:07:40 UTC
Created attachment 82662 [details]
A relatively long markings file
Comment 2 Vassilis Palassopoulos 2013-10-04 17:09:30 UTC
Comment on attachment 82662 [details]
A relatively long markings file

A subset of the markings file I have for installing my preferred applications on a fresh install
Comment 3 Vassilis Palassopoulos 2013-10-04 17:11:05 UTC
Created attachment 82665 [details]
A smaller version of the markings file (bug doesn't appear)

A subset of test-small on which the bug doesn't appear.
Comment 4 Vassilis Palassopoulos 2013-10-04 17:12:54 UTC
Created attachment 82666 [details]
A smaller version of the markings file (bug appears)

Almost the same as test-small-works with the inclusion of one additional package. With that small change, the bug appears
Comment 5 Vassilis Palassopoulos 2013-10-04 17:19:35 UTC
For reasons of reproducibility and testing I attached a set of files (markings files) on which the bug appears (or not):

A relatively long markings file: This is a subset of the markings file I have for installing my preferred applications on a fresh system.
A smaller version of the markings file (bug doesn't appear): This is a subset of the previous markings file on which the bug doesn't appear.
A smaller version of the markings file (bug appears): This is almost identical with the previous markings file with the difference of the inclusion of one additional package install. With that small change, the bug appears.
Comment 6 Vassilis Palassopoulos 2013-10-14 09:02:12 UTC
After reading the code of bool Backend::loadSelections(const QString &path) and comparing it with bool RPackageLister::readSelections(istream &in) (http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/raring/synaptic/raring/view/head:/common/rpackagelister.cc) from Synaptic (which uses the same basic algorithm) I think I found the bug inside a while loop:

if (pkgIter.end()) {
    return false;
}

When pkgIter.end() becomes true, the method interrupts and returns false. What should happen is check when pkgIter.end() becomes false in order to execute the remaining commands in the while loop. In the case that it becomes true it should simply iterate mapIter and the while loop.

I'll upload a patch with the fix.
Comment 7 Vassilis Palassopoulos 2013-10-14 09:08:02 UTC
Created attachment 82838 [details]
Don't interrupt method when pkgIter.end() becomes true
Comment 8 Vassilis Palassopoulos 2013-10-16 08:48:02 UTC
The aforementioned reason for existence of this bug alongside with the patch that suggests how it could be fixed, suggests that in some cases the method interrupts while partially executing some of the commands (of marking some package changes). After testing it more, I found a use case where this happens (some of the markings in the markings file don't get processed as the method interrupted before they got processed).

For that reason this new finding suggests that the bug is more serious than was initially thought.

I will upload a markings file with the case that some markings don't get processed.
Comment 9 Vassilis Palassopoulos 2013-10-16 08:49:21 UTC
Created attachment 82872 [details]
A markings file where not all markings get processed
Comment 10 Rohan Garg 2013-11-11 11:15:40 UTC
Hiya
I suppose you're running Saucy? Assuming that you are, then, the adobe-flash-properties-kde package doesn't exist in Saucy ( infact the entire adobe-flashplugin source was not built for saucy ).

So the error is actually a valid one since Muon could not mark the adobe-flash-properties-kde package for installation since it doesn't exist.

While your patch is fine, I think it would be better to throw a more meaningful error like "Could not mark package foo for installation/removal" rather than just continue with the packages it successfully marked.
Comment 11 Rohan Garg 2013-11-11 11:57:43 UTC
OTOH this also exposes a bug where libqapt doesn't ignore a line starting with tab.
Comment 12 Vassilis Palassopoulos 2013-11-11 12:04:15 UTC
Hi,

I tested it in both Raring and Saucy (when it was beta 2) so I didn't notice this package was missing in Saucy.

I'd like to mention though that this package might not be the only reason this error arises.

Take a look at "A smaller version of the markings file (bug appears)" and "A smaller version of the markings file (bug doesn't appear)". I've checked those packages and they are all available on Saucy. This makes me believe the error is not valid.
Comment 13 Rohan Garg 2013-11-11 12:13:22 UTC
I tried out http://pastebin.kde.org/prslwawfnwith "A smaller version of the markings file (bug appears)" and it works for me. Could you check if that works for you?
Comment 14 Rohan Garg 2013-11-11 12:13:59 UTC
Ugh, bugzilla ate a space, the link is supposed to be http://pastebin.kde.org/prslwawfn
Comment 15 Rohan Garg 2013-11-11 12:29:33 UTC
Before commiting this, I'd like a justification from you as to why you use tab's to add package descriptions instead of using a '#' which is the supported method.
Comment 16 Vassilis Palassopoulos 2013-11-11 17:57:59 UTC
I checked again if it works for me. Currently it doesn't work on what ships on Saucy.

I can't check if it works with the change because I am having a difficulty when building Muon (it builds against the installed libQApt instead the one I cloned from the git repo and applied the patch, tried to change that with no luck (I'm new with cmake)). For the same reason I wasn't able to test the patch I made.

The usage of tabs instead of the usage of '#' was purely for aesthetics.
I was previously using Synaptic where it worked just fine and afterwards I found that it works on Muon on some circumstances as well (e.g. the bug doesn't appear file).

So if the problem persists only with files that use tabs for comments instead of '#' it's just a cosmetic issue and the only real reason I could see it as important is purely for reasons of interoperability between Synaptic and Muon.

At this point I'd like to remind that there is also a command on dpkg that does the same job: dpkg --set-selections < selection.txt
I haven't checked if it supports files that use tabs for comments and I don't know in general how interoperable set-selections is with Muon and Synaptic. The more interoperability the better of course.

For my personal usage it doesn't matter if the comment has to start with '#' instead of a tab.
I tested again my complete markings file (adding '#' at the start of every comment and removing the flash package this time as you suggested (didn't notice this at first)) and it worked as expected.

So there are two things

1. Should Muon report which markings it couldn't process, as you suggested, than simply aborting the process with a general message? Should it process the other markings as well in the case it couldn't process a subset of them?

2. Does Muon have it's own standard of markings file format or does it try to be interoperable with other programs? Which ones? Synaptic? dpkg?

In any case currently when it pops up the error message, it processes some markings, although it says it couldn't process them (implying it couldn't process any of them) and the GUI behaves awkwardly (as previously explained). Additionally the undo/redo functionality gets messed up a little bit. For example if it tries to read blank markings files, it'll create new states of changes (switching between them with undo/redo) while actually nothing changes! (since the states are void)

Since I see the main dev is not available, maybe the best choice is the least time consuming effort. This would be changing the error message to "Could not mark some changes. Please make sure that the file is a markings file created by either the Muon Package Manager or the Synaptic Package Manager". This partially fixes the bug as it warns that only some changes couldn't be marked while the GUI awkwardness persists.
Comment 17 Justin Zobel 2021-03-10 00:12:36 UTC
Thank you for the bug report.

As this report hasn't seen any changes in 5 years or more, we ask if you can please confirm that the issue still persists.

If this bug is no longer persisting or relevant please change the status to resolved.
Comment 18 Vassilis Palassopoulos 2021-03-10 10:11:34 UTC
I am not a user of the Muon Package Manager any more thus this issue does not affect me anymore.