bundle setup.py install with --destdir

[quozl]

Debian bugs 975373 and 975374 report that the desktop files installed by Debian packages of Sugar activities contain build directory paths when they should not.

In turn this is caused by /usr/share/cdbs/1/class/python-sugar.mk calling setup.py install with --prefix set to the DEB_DESTDIR variable.

In turn this is caused by the Sugar Toolkit providing PREFIX support but not DESTDIR support.

We could add DESTDIR support, using a --destdir option.

[chimosky]

What path would be best for DESTDIR? Looking at the thread, the suggestion was the problem would be fixed in _install_desktop_file as one of the arguments was prefix and from what you said above setup.py is called with --prefix set to DEB_DESTDIR variable, how would adding a --destdir option change setup.py being called with --prefix.

Can you help me understand the perl line in?

find $(cdbs_curdestdir)/usr/share -type f -name '*.desktop' -execdir perl -pi -e 's,^\S+\s*=\s*\K\S*(?=/usr/share),,g' '{}' ';'

[quozl]

Thanks for looking into this. As far as I know, it only impacts Debian Developers and people who use our source distribution.

What path would be best for DESTDIR?

In testing, a good path would be /tmp/something. I've done this with --prefix using our setup.py, and the desktop files do indeed contain the path, as they should; compliant with prefix convention.

Looking at the thread, the suggestion was the problem would be fixed in _install_desktop_file as one of the arguments was prefix and from what you said above setup.py is called with --prefix set to DEB_DESTDIR variable, how would adding a --destdir option change setup.py being called with --prefix.

_install_desktop_file would have to change as well, to use --destdir instead of --prefix. A Debian Developer would be responsible for that once the toolkit grows the feature. We could make it easier for them by including that in the release notes.

Can you help me understand the perl line in?

find $(cdbs_curdestdir)/usr/share -type f -name '*.desktop' -execdir perl -pi -e 's,^\S+\s*=\s*\K\S*(?=/usr/share),,g' '{}' ';'

If I had the time, I'd install packages perl-doc or perl-doc-html and look at man perlreqquick and man perlretut. The introducing comment in the Write activity packaging says "Fix broken paths in desktop file", so presumably it looks for lines containing something equals something with /usr/share and strips them of any path components prior to /usr/share. In testing, that's what it seems to do; thus;

$ echo Icon=/build/1st/sugar-read-activity-123/debian/sugar-read-activity//usr/share/sugar/activities/Read.activity/activity/activity-read.svg > test.desktop
$ perl -pi -e 's,^\S+\s*=\s*\K\S*(?=/usr/share),,g' test.desktop
$ cat test.desktop 
Icon=/usr/share/sugar/activities/Read.activity/activity/activity-read.svg

However, this is only of minor interest; we can't use this workaround inside the toolkit, because we don't know that /usr/share will be used by every user of the source.

Instead, we should add --destdir option, and implement it according to the convention.

[chimosky]

Thanks for the explanation above, it made sense.

I agree with adding a --destdir option but I also noticed that activity_path was set to a value with prefix being the first and that explains the problem above, I'm thinking in addition to adding a --destdir option we should make sure prefix is sys.prefix as prefix is still important and we can't discard it in install().

[quozl]

I've not fully worked through it, or tested it, but this might be progress;

--- a/src/sugar3/activity/bundlebuilder.py
+++ b/src/sugar3/activity/bundlebuilder.py
@@ -280,10 +280,12 @@ class Installer(Packager):
         Packager.__init__(self, builder.config)
         self.builder = builder

-    def install(self, prefix, install_mime=True, install_desktop_file=True):
+    def install(self, prefix, destdir,
+                install_mime=True, install_desktop_file=True):
         self.builder.build()

-        activity_path = os.path.join(prefix, 'share', 'sugar', 'activities',
+        activity_path = os.path.join(destdir, prefix,
+                                     'share', 'sugar', 'activities',
                                      self.config.bundle_root_dir)

         source_to_dest = {}
@@ -316,8 +318,12 @@ class Installer(Packager):
             self.config.bundle.install_mime_type(self.config.source_dir)

         if install_desktop_file:
-            self._install_desktop_file(prefix, activity_path)
-            self._generate_appdata(prefix, activity_path)
+            activities = os.path.join(prefix,
+                                      'share', 'sugar', 'activities',
+                                      self.config.bundle_root_dir)
+
+            self._install_desktop_file(destdir, activities)
+            self._generate_appdata(destdir, activities)

     def _install_desktop_file(self, prefix, activity_path):
         cp = ConfigParser()
@@ -506,6 +512,7 @@ def cmd_install(config, options):
     installer = Installer(Builder(config))
     installer.install(
         options.prefix,
+        options.destdir,
         options.install_mime,
         options.install_desktop_file)

@@ -592,6 +599,9 @@ def start():
     install_parser.add_argument(
         "--prefix", dest="prefix", default=sys.prefix,
         help="Path for installing")
+    install_parser.add_argument(
+        "--destdir", dest="destdir", default=sys.prefix,
+        help="Path for staged install")
     install_parser.add_argument(
         "--skip-install-mime", dest="install_mime",
         action="store_false", default=True,

What do you think?

[quozl]

https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/454 for testing and review.

[chimosky]

What do you think?

Your PR looks good, I was having doubts about making destdir part of activity_path as I didn't see a need for that and --destdir defaulting to sys.prefix didn't seem right too.

Let me test the commit.