Comment 11 for bug 1549942

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.

> 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).

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.

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.

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.

 - 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.

 - 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'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.

> 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.

  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.)