Merge lp:~mapreri/inkscape/support-scour-0.26 into lp:~inkscape.dev/inkscape/trunk

[Mattia Rizzolo]

Originally reported at https://bugs.debian.org/852290

When saving as an optimised SVG, inkscape uses the scour library to
optimise the SVG. An error is reported when doing so, triggered by
importing `scourString` from `scour.scour`.

The problem is most likely a result of a change in the python-scour
package:

- In package version 0.26-3 (jessie), scour.py exists as:
  /usr/lib/python2.7/dist-packages/scour.py
- In package version 0.32-2 (stretch), scour.py exists as:
  /usr/lib/python2.7/dist-packages/scour/scour.py

[Patrick Storz (ede123)]

While the diff looks good to me (not tested yet, though):

Do we really need to support scour 0.26?
It's ancient and many bugs have been fixed in the meantime as well as new features added.

0.26 is the last version that was released before moving the source code to GitHub, re-packaging the module properly and switching to pypi as the preferred distribution method.

As an active Scour developer and the one responsible for the recent update in Inkscape I'd very much prefer if we could always refer people to newer versions and if Linux distros could catch up and bundle recent versions of the Scour module.

In my opinion it is an unnecessary burden to try and maintain compatibility with the old version when we don't even want people to use it.

[Mattia Rizzolo (mapreri)]

Yes I see your point.

Debian has "catch up" with scour 0.28 back in 2015 already, this is about supporting the already released (back then) Debian jessie, which was released in April 2015 with scour 0.26. Now, I know that the Debian update came late (as way too often is the case…), but well, Debian jessie will be supported for only about one more year, and backporting the newer scour is actually more work than this quite small patch.

If you prefer to do not support older scour, despite the easiness of doing it, I'd just stitch this patch in the backported inkscape for the time being (I'm going to keep maintaining only until Debian stretch is released, expected to be in the next very few months).

[Patrick Storz (ede123)]

I certainly would prefer not to support older Scour but if this means it's inaccessible to some people it's obviously worse.

Would it be viable to land this only as a patch for Debian? Are any other distros affected that would warrant having this in the official repo?

If you'd prefer to have it in the Inkscape repo the question is also whether merging in trunk is the way to go (I guess 0.92.x would make more sense? Will Inkscape 0.93 ever be available for Debian Jessie while it's supported?)

In either case I noticed one problem with the patch:
You only need to check if you can "import scour" once (it's the same for 0.26 as well as newer versions).
You should then in a second check decide from where to import "scourString".

[Mattia Rizzolo (mapreri)]

On Mon, Jan 23, 2017 at 07:04:34PM -0000, Eduard Braun wrote:
> Would it be viable to land this only as a patch for Debian?

Yes, it's surely viable (we have quite a good support for carrying
out-of-tree patches in debian packages)
Though I'd probably drop it once I'll stop dealing with that old
release, which will probably be in few months anyway, since I clearly
prefer to have as fewer modifications from upstream as possible.

> Are any other distros affected that would warrant having this in the official repo?

I don't really know.
I only deal with Debian and Ubuntu. I can tell you that Ubuntu trusty
(14.04) also has scour 0.26. I don't provide backports for that, but
the official PPAs in
https://launchpad.net/~inkscape.dev/+archive/ubuntu/stable/+packages and
https://launchpad.net/~inkscape.dev/+archive/ubuntu/trunk/+packages do,
and clearly that feature is broken too there.

I'll let the inkscape people (and you) decide whether such support is
important enough.

> If you'd prefer to have it in the Inkscape repo the question is also
> whether merging in trunk is the way to go (I guess 0.92.x would make
> more sense? Will Inkscape 0.93 ever be available for Debian Jessie
> while it's supported?)

It's not only "while it's supported". Clearly I would not provide 0.93
in jessie-backports-sloppy by myself; but given that doing so would not
be hard at all (thank you for not tightening the dependencies!), if a
user was ever to ask for it I'd be inclined grant the wish.

> In either case I noticed one problem with the patch:
> You only need to check if you can "import scour" once (it's the same for 0.26 as well as newer versions).
> You should then in a second check decide from where to import "scourString".

right.
I don't think that's an actual problem though (worse case it'd lead to
duplicated a duplicated import, which python just ignores/deal
gracefully with).

[Patrick Storz (ede123)]

I was recently working on the Scour extension (see [1] and [2]) and also pushed [3] in the process which should fix this issue.

I'll probably not push it to 0.92.x stable branch as it includes a lot of new strings, but I might include the simple import fix once 0.92.1 is out (I'm afraid I lingered too long and we're now in hard freeze).

[Patrick Storz (ede123)]

[1] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15482
[2] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15483
[3] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15484

[Mattia Rizzolo (mapreri)]

Thanks, I've picked 15484 instead of my solution.

Feel free to decline this merge request, then :) (I seem to not be able to do it myself…)

[Patrick Storz (ede123)]

Fix now also committed to 0.92.x branch in r15377
http://bazaar.launchpad.net/~inkscape.dev/inkscape/0.92.x/revision/15377

[Mattia Rizzolo (mapreri)]

Great, thank you!

On Thu, Feb 16, 2017 at 10:23 PM Eduard Braun <email address hidden> wrote:

> The proposal to merge lp:~mapreri/inkscape/support-scour-0.26 into
> lp:inkscape has been updated.
>
> Status: Needs review => Merged
>
> For more details, see:
>
> https://code.launchpad.net/~mapreri/inkscape/support-scour-0.26/+merge/315348
> --
> You are the owner of lp:~mapreri/inkscape/support-scour-0.26.
>
> Launchpad-Message-Rationale: Owner
> Launchpad-Message-For: mapreri
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~mapreri/inkscape/support-scour-0.26
> Launchpad-Project: inkscape
>

=== modified file 'share/extensions/scour.inkscape.py'
--- share/extensions/scour.inkscape.py	2016-01-12 22:54:44 +0000
+++ share/extensions/scour.inkscape.py	2017-01-23 12:11:44 +0000
@@ -2,20 +2,34 @@
 # -*- coding: utf-8 -*-
 import sys, platform, inkex
 
+def import_error(module, err):
+    inkex.errormsg(
+        "Failed to import Python module '%(module)s'.\n"
+        "Please make sure it is installed (e.g. "
+        "using 'pip install %(module)s' "
+        "or 'sudo apt-get install python-%(module)s') and try again."
+        % {"module": module}
+    )
+    inkex.errormsg("\nDetails:\n" + str(err))
+    sys.exit()
+
 try:
     import scour
     from scour.scour import scourString
+except ImportError:
+    # scour <= 0.26
+    try:
+        import scour
+        from scour import scourString
+    except Exception as e:
+        import_error("scour", e)
 except Exception as e:
-    inkex.errormsg("Failed to import Python module 'scour'.\nPlease make sure it is installed (e.g. using 'pip install scour' or 'sudo apt-get install python-scour') and try again.")
-    inkex.errormsg("\nDetails:\n" + str(e))
-    sys.exit()
+    import_error("scour", e)
 
 try:
     import six
 except Exception as e:
-    inkex.errormsg("Failed to import Python module 'six'.\nPlease make sure it is installed (e.g. using 'pip install six' or 'sudo apt-get install python-six') and try again.")
-    inkex.errormsg("\nDetails:\n" + str(e))
-    sys.exit()
+    import_error("six", e)
 
 class ScourInkscape (inkex.Effect):