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 > > <steve.langa...@canonical.com> 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! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1549942 Title: php-imagick failing autopkgtests To manage notifications about this bug go to: https://bugs.launchpad.net/imagemagick/+bug/1549942/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
