xslate / p5-Mouse

Lightweight class builder for Perl, as a subset of Moose
https://metacpan.org/release/Mouse
Other
46 stars 32 forks source link

Fix thread issue for Perl 5.22 or higher #50

Closed syohex closed 9 years ago

syohex commented 9 years ago

Please review this change. I don't understand XS well.

I confirmed that test of #29 and tests of Text::Xslates are passed with this change. I fixed by reference to Class::Accessor::Inherited::XS.

This is related to #29, #49.

syohex commented 9 years ago

Tests of Text::Xslate are failed with original code. (docker build with following Dockerfile is failed)

FROM perl:5.22.0-threaded

RUN cpanm -v Mouse
RUN cpanm -v Text::Xslate

All tests of Text::Xslates are passed with Mouse with this change. (docker build with following Dockerfile is success)

FROM perl:5.22.0-threaded

RUN curl -LO https://cpan.metacpan.org/authors/id/G/GF/GFUJI/Mouse-2.4.2.tar.gz \
    && tar zxvf Mouse-2.4.2.tar.gz \
    && cd Mouse-2.4.2 \
    && curl -LO https://github.com/gfx/p5-Mouse/pull/50.patch \
    && patch -p1 < 50.patch \
    && cpanm --installdeps . \
    && perl Build.PL \
    && ./Build \
    && ./Build test \
    && ./Build install

RUN cpanm -v Text::Xslate

(I suppose these Dockerfiles are not good, I don't understand Docker well :-()))

gfx commented 9 years ago

Looks good. Can you add perl 5.22 to .travis.yml?

syohex commented 9 years ago

Done

syohex commented 9 years ago

:scream: Perl 5.22.0 is not available yet.

syohex commented 9 years ago

I reverted .travis change

gfx commented 9 years ago

:(

pghmcfc commented 9 years ago

The use of mg_findext in this update brings some issues for older perls:

  1. The Devel::PPPort dependency needs to be bumped to 3.22, where this function became supported
  2. A couple of tweaks to mouse.h are also needed for the backport to work:
--- mouse.h
+++ mouse.h
@@ -1,6 +1,8 @@
 #ifndef MOUSE_H
 #define MOUSE_H

+#define NEED_mg_findext
+
 #define PERL_EUPXS_ALWAYS_EXPORT

 #include "xshelper.h"
@@ -111,6 +113,7 @@ static inline MAGIC *MOUSE_get_magic(CV
 #ifndef MULTIPLICITY
     return (MAGIC*)(CvXSUBANY(cv).any_ptr);
 #else
+    dTHX;
     return mg_findext((SV*)cv, PERL_MAGIC_ext, vtbl);
 #endif
 }

I'm by no means an expert here but these changes worked for me in a variety of older environments, at least as far as passing the test suite.

syohex commented 9 years ago

@pghmcfc Do you have the issue with Mouse v2.4.4 ? Mouse v2.4.3 have the issue which you indicated. I suppose it is fixed at Mouse v2.4.4.

pghmcfc commented 9 years ago

@syohex, only the issue regarding the need for dTHX is fixed in v2.4.4; the Devel::PPPort version requirement still needs increasing from 3.19 to 3.22 and NEED_mg_findext has to be defined where ppport.h support for mg_findext is needed, for older perls that don't have it (prior to 5.13.8 I think).

syohex commented 9 years ago

@pghmcfc I fixed at the issue and released v2.4.5. Please check it.

pghmcfc commented 9 years ago

@syohex, works for me, thanks.