On 07.03.2016 [03:57:59 -0000], Steve Langasek wrote:
> On Sun, Mar 06, 2016 at 07:02:56PM -0000, Nish Aravamudan wrote:
> > On Sun, Mar 6, 2016 at 12:57 AM, Steve Langasek
> > <email address hidden> wrote:
> > >>    - d/tests/control: limit imagemagick to 1 thread to avoid openmp
> > >>      related segfaults.
> 
> > I'm actually following the recommendations upstream for php-imagick
> > (https://github.com/mkoppanen/imagick#openmp). I believe this is
> > already a known issue with the imagemagick/openmp code and the
> > php-imagick maintainer implies this is where they spend a lot of their
> > time (https://github.com/mkoppanen/imagick/issues/138).
> 
> The imagemagick code isn't what has changed here; php-imagick has changed. 
> The php-imagick package in xenial has passed its test suite with all recent
> versions of imagemagick in the archive, but the new php-imagick (which
> includes substantial code changes) is failing.

Right, you are correct that the variable here is php-imagick. Sorry for
mis-stating that earlier.

For reference, I'm actually not sure why, but Debian's version has been
failing for much longer
(https://ci.debian.net/packages/p/php-imagick/unstable/amd64/). And that
occurred with different versions of both php-imagick and imagemagick.

What can be happening here is that php-imagick relies on some behavior
(php-imagick upstream is ony tested against official imagemagick
releases) that is supposed to be present in some version of imagemagick
(checked by version string). Debian/Ubuntu backport patches that alter
functionality but do not bump the version string and that can lead to
php-imagick not doing the right thing. I'm not sure if that is the case
currently.

> > The bug is in imagemagick, though, not in php-imagick.
> 
> First, this is not at all clear to me.  Imagemagick is a widely used utility
> and library; there have been no changes to the upstream version of
> imagemagick included in Ubuntu since 15.04; and there are no reports of
> segfaults with imagemagick outside of php-imagick (except for input
> fuzzing).  This suggests to me that this is not a bug in imagemagick at all,
> but a misuse of the imagemagick API by php-imagick (possibly related to
> competing threading implementations in PHP vs. libmagick).

Well, I am new to this, but my understanding of php-imagick is that
there have been many bugs fixed in imagemagick's memory handling (and
threading, possibly) due to php-imagick. Not all of those fixes are in
Debian & Ubuntu's imagemagick.

As indicated by my backports already in this bug (and in my latest),
there are clearly missing upstream changes that are needed in the
Debian/Ubuntu version of imagemagick (both an incomplete backport and a
(possibly) missing one). Note that if I hold php-imagick constant and
test against the two version of imagemagick (with and without my fixes),
I get different results (while upstream php-imagick against upstream
imagemagick does not see any issues) -- that to me indicates the bug is
missed fixes in imagemagick. So I'm not entirely sure whether it's a
misuse of an API (which seems like it'd be broken in upstream
php-imagick against our base version of imagemagick) or if it's a broken
(or not thread-safe) API in our version of imagemagick that has been
fixed upstream.

I believe there was also, actually, an upstream release of 6.8.9.10
based upon my reading of their git logs. But not sure.

> Second, even should this be a bug that needs to be fixed in imagemagick, it
> is the *combination* of imagemagick and php-imagick that is buggy.  The test
> suite is currently giving us a strong indication that whereas the current
> php-imagick package is usable, upgrading to the new php-imagick package will
> make it substantially less usable.

Again, I know that seems to be the case, but Debian's results imply
something else. Are the launchpad sbuilds multi-cpu? This is actually
somewhat concerning. I consistently get segmentation fault failures from
the tests in my runs -- but the launchpad build logs do not indicate
any. Debian's, as I mentioned, do.

Another big difference between the version of php-imagick, ignore
functionality, is a huge increase in coverage tests. I belive
php-imagick 3.3.0~rc2 only runs 27 tests, while 3.4.0~rc6 runs 251. More
on this below.

> We should not make the test suite artificially pass if it's giving us
> accurate information about the (non-)usability of the package.  The only
> reason to make this change to the test suite would be if the crasher bugs it
> exposes are *not* real bugs and do not affect production use of the package. 
> This seems unlikely to be the case.  But if you've analyzed the failures and
> found this to be true, please let me know.

That's a good point. I did not intend my fix to be a long-term solution,
and should have made that clear in my changelog entry.

> The other possibilities here are:
> 
>  - The old php-imagick in xenial is already buggy and crashes in the same
>    way, but the testsuite for the old version of the package is
>    insufficiently rigorous to pick up on it.  In this case, it's appropriate
>    to override the testsuite failures *in proposed-migration* to let the new
>    package in because this is a test regression but not a functional
>    regression.

I think this is the case, to be honest, based upon my interaction with
the upstream maintainer and my own testing results.

>  - The old php-imagick in xenial is not buggy in this way and this
>    represents a real regression in the runtime behavior of the package.  In
>    this case, either we need a real fix to the code, so that the testsuite
>    passes *without* a testsuite-specific workaround; or we should to remove
>    the php-imagick package from xenial as too-buggy-to-live, and let it back
>    in only when it's resolved.

My impression from the upstream maintainer is this racy/buggy behavior
with imagemagick is not new.

>  - The old php-imagick in xenial is not buggy, this is a real regression in
>    the runtime behavior, the package cannot be removed (because it has too
>    many reverse dependencies), and we can't fix the runtime behavior in a
>    reasonable amount of time for the php transition.  In this case, we could
>    force the package migration in proposed-migration, *not* by applying a
>    testsuite workaround; and should treat this as a high-priority bug to be
>    fixed before release.

I don't think that is the case here.

> I've had a look at the ImageMagick code to see what MAGICK_THREAD_LIMIT=1
> does, and see that the same thing can be accomplished programmatically by
> calling SetMagickResourceLimit(ThreadResource, 1).  I believe that invoking
> this from the php-imagick MINIT function should be sufficient to get correct
> behavior at runtime, not just in the testsuite; I will prepare a patch here
> for testing.
> 
> > Alternatively, php-imagick *could* change the configuration of
> > imagemagick when installed. But I don't think that is normal or
> > possibly even allowed by the Debian manual.
> 
> Indeed not - but it is appropriate for php-imagick to configure the behavior
> of the library at runtime for its own usage.

That's fair. I think the issue is that *some* imagemagick installations
are fine with openmp enabled. But many are not (and I'm not sure if it's
heavily tested, per se -- I really don't know, I understand imagemagick
tests it, and I understand that we run the test suite for imagemagick,
but I don't know what coverage that represents for these paths).

> > The underlying issue here is that php-imagick and imagemagick need to
> > be kept in sync -- any modification of one must be tested against the
> > other for regressions (and that is not currently done, afaict). So
> > when backports to imagemagick occur, they are self-consistent (as far
> > as imagemagick's own tests go), but consumers of the API seem to break
> > (php-imagick's tests).
> 
> Changes in one *are* tested against the other - via autopkgtest, in
> xenial-proposed, and regressions block the migration of the corresponding
> package to the xenial pocket.  Your proposed patch would route around this
> sanity check.

You're right, sorry -- the issue here is that the tests in the old
version of php-imagick were insufficient to catch the broken backport in
the latest version of imagemagick.

>   http://autopkgtest.ubuntu.com/packages/p/php-imagick/xenial/amd64/
> 
> (I'm not sure why the latest imagemagick upload is only showing test results
> against the xenial-proposed version of php-imagick instead of the xenial
> version; but you can see that php-imagick 3.3.0~rc2-1 in xenial *did* pass
> with imagemagick 8:6.8.9.9-7.)

Which is really confusing to me, because that combination *didn't* pass
in Debian:
https://ci.debian.net/data/packages/unstable/amd64/p/php-imagick/20160218_021453.autopkgtest.log.gz

Thanks for all your help, Steve!