Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix thread issue for Perl 5.22 or higher #50

Merged
merged 2 commits into from
Jun 29, 2015
Merged

Fix thread issue for Perl 5.22 or higher #50

merged 2 commits into from
Jun 29, 2015

Conversation

syohex
Copy link
Collaborator

@syohex syohex commented Jun 24, 2015

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.

In original code, mismatching plan error is occurred.
@syohex syohex changed the title Fix thread issue for Perl 5.20 or higher Fix thread issue for Perl 5.22 or higher Jun 24, 2015
@syohex
Copy link
Collaborator Author

syohex commented Jun 25, 2015

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
Copy link
Member

gfx commented Jun 29, 2015

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

@syohex
Copy link
Collaborator Author

syohex commented Jun 29, 2015

Done

@syohex
Copy link
Collaborator Author

syohex commented Jun 29, 2015

@syohex
Copy link
Collaborator Author

syohex commented Jun 29, 2015

I reverted .travis change

@gfx
Copy link
Member

gfx commented Jun 29, 2015

:(

gfx added a commit that referenced this pull request Jun 29, 2015
Fix thread issue for Perl 5.22 or higher
@gfx gfx merged commit e674baa into master Jun 29, 2015
@gfx gfx deleted the fix-thread-issue branch June 29, 2015 11:20
syohex added a commit that referenced this pull request Aug 2, 2015
Changelog diff is:

diff --git a/Changes b/Changes
index 3459479..96ccf12 100644
--- a/Changes
+++ b/Changes
@@ -2,6 +2,9 @@ Revision history for Mouse

 {{$NEXT}}

+v2.4.3 2015-08-02T07:10:09Z
+    - Fix for Perl 5.22.0 or higher(#50)
+
 2.4.2 2015-04-12T01:22:02Z
     - Fixed #40; 'use strict' not enabled when 'use 5.010' follows 'use Mouse'
     - Fixed #39; New warnings in Perl 5.21.x: redundant arguments for sprintf
syohex added a commit that referenced this pull request Aug 2, 2015
Changelog diff is:

diff --git a/Changes b/Changes
index 96ccf12..bd3654d 100644
--- a/Changes
+++ b/Changes
@@ -2,6 +2,9 @@ Revision history for Mouse

 {{$NEXT}}

+v2.4.4 2015-08-02T09:49:34Z
+    - Fix for older Perl(< 5.22.0) (#53)
+
 v2.4.3 2015-08-02T07:10:09Z
     - Fix for Perl 5.22.0 or higher(#50)
@pghmcfc
Copy link

pghmcfc commented Aug 2, 2015

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
Copy link
Collaborator Author

syohex commented Aug 2, 2015

@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
Copy link

pghmcfc commented Aug 3, 2015

@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
Copy link
Collaborator Author

syohex commented Aug 4, 2015

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

@pghmcfc
Copy link

pghmcfc commented Aug 4, 2015

@syohex, works for me, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants