Merge lp:~brunonova/software-properties/lp1381050 into lp:software-properties

[Bruno Nova]

This is a fix/workaround for Bug #1381050, and was tested in Ubuntu (only tested GTK version!).

When importing a key file that is located in a path that contains special characters (example: /home/USER/Transferências/Release.key), it fails silently, throwing an "org.freedesktop.DBus.Python.UnicodeEncodeError" exception.

It seems that the Software Properties DBus service (or the whole DBus) is NOT running in a UTF-8 locale, causing this issue.

This commit adds a line that forces the file path in the DBus service to be encoded in UTF-8.

If DBus is indeed running in a non-UTF-8 locale (maybe C?), this needs to be further investigated as it may be causing other encoding errors elsewhere!

[Michael Vogt (mvo)]

review: Approve

[Michael Vogt (mvo)]

Thanks for this branch. I merged it initially and then added a test for the add_key() funtion in http://bazaar.launchpad.net/~ubuntu-core-dev/software-properties/main/revision/922 and simplified it.

[Bruno Nova (brunonova)]

@Michael, are you sure the simplification works?
I applied those changes to the SoftwareProperties.py file installed in my Trusty system (without any other changes), killed the running software-properties-dbus proccess then tried adding a key from my ~/Transferências folder and it didn't work.
I also started a Vivid virtual machine, grabbed the latest version from lp:software-properties, built and installed the debian package, and it also didn't work there.
Could you check that out? I may be doing something wrong, of course!

The other drag-drop bug I reported seems to be fixed, though.

[Michael Vogt (mvo)]

@Bruno: Thanks for double checking. I just wrote a testcase to verify, so if its not working for you there is maybe more going on.

[Bruno Nova (brunonova)]

I just checked the KDE version to see if it works.
When I add or remove a key, an error is displayed, but it seems that the key is actually added/removed.
software-properties-kde must be run as root, and doesn't seem to use DBus.

I investigated a bit.
These actions call the KeysModified() method. This method raises an exception in the KDE version... because it doesn't exist in this version! And it seems that the only thing that the method does is log a debug message.
I'll probably report a new bug for this issue.

Also, from the tests I made, the changes in this branch and in the other (drag&drop bug) should not break the KDE version.

[Bruno Nova (brunonova)]

OK, reported Bug #1398180.

(Oh, and KeysModified() does more in the GTK version than logging a debug message).

[Bruno Nova (brunonova)]

@Michael
I completely forgot about these bugs!

I cloned/branched lp:software-properties and tested it, and the fix seems to be working correctly.
My test: I killed the running software-properties-dbus process then started, as root, the fixed one from the cloned branch. Then I ran software-properties-gtk and added/removed the VLC key from my "~/Transferências" folder, and it worked.
I also tested lp:ubuntu/software-properties successfully.
So, either you fixed the issue I mentioned, or I was wrong. :)

Also, bug #1383289 is fixed, so could you mark it as fixed (it wasn't marked automatically).
And we should backport these fixes to Utopic and Trusty.

[Bruno Nova (brunonova)]

I marked the bug as "Fixed Released" myself.

[Michael Vogt (mvo)]

On Thu, Jan 29, 2015 at 12:29:09PM -0000, Bruno Nova wrote:
> @Michael
> I completely forgot about these bugs!
>
> I cloned/branched lp:software-properties and tested it, and the fix seems to be working correctly.
> My test: I killed the running software-properties-dbus process then started, as root, the fixed one from the cloned branch. Then I ran software-properties-gtk and added/removed the VLC key from my "~/Transferências" folder, and it worked.
> I also tested lp:ubuntu/software-properties successfully.
> So, either you fixed the issue I mentioned, or I was wrong. :)

Thanks a lot for confirming, I'm too lazy^Wbusy to check what fixed
it, but I'm very happy its fixed :)

> Also, bug #1383289 is fixed, so could you mark it as fixed (it wasn't marked automatically).
> And we should backport these fixes to Utopic and Trusty.

I created a trusty task, not sure about utopic. If you have time, it
would be awesome if you could create a diff for a SRU upload. I'm
happy to sponsor the upload but unfortunately I'm pretty busy right
now so not much time to prepare/test a diff myself :/

Thanks!
 Michael

[Bruno Nova (brunonova)]

> Thanks a lot for confirming, I'm too lazy^Wbusy to check what fixed
> it, but I'm very happy its fixed :)

> I created a trusty task, not sure about utopic. If you have time, it
> would be awesome if you could create a diff for a SRU upload. I'm
> happy to sponsor the upload but unfortunately I'm pretty busy right
> now so not much time to prepare/test a diff myself :/

Haha, I imagine how busy you guys must be, with all the stuff you're doing at the same time! :)
I'll make a branch for trusty-proposed for #1381050 and #1383289 when I have time.

Could you also create a Trusty task for bug #1381050? The two bugs mentioned in the previous paragraph are related (the second was found when investigating the first, and were fixed at the same time; additionally, #1381050 is the actual bug related to this merge request).

About Utopic, I think it's still affected by these two bugs.
Maybe it should also receive the backported fixes, but it's not my decision. :)

=== modified file 'softwareproperties/SoftwareProperties.py'
--- softwareproperties/SoftwareProperties.py	2014-09-24 09:50:37 +0000
+++ softwareproperties/SoftwareProperties.py	2014-10-20 12:49:14 +0000
@@ -799,6 +799,7 @@
     """Add a gnupg key to the list of trusted software vendors"""
     if not isinstance(path, str):
       path = str(path) # allows non-ascii filenames
+    path = path.encode('utf-8') # ensure path is encoded in UTF-8
     if not os.path.exists(path):
         return False
     try: