Dual-Life / Devel-PPPort

Perl/Pollution/Portability
9 stars 28 forks source link

gv_fetchpvn_flags() is now supported? #42

Closed wyoung closed 8 years ago

wyoung commented 8 years ago

While chasing a different problem I seem to have stumbled across a documentation bug. According to man Devel::PPPort, gv_fetchpvn_flags() is unsupported, but on grepping the source code, I find that there is an implementation, and that the text of that routine is built into the local PPPort.so here on Perl 5.8.8.

Did it get implemented and someone forgot to remove this item from the manual page?

wyoung commented 8 years ago

I see that this was added in 3.33.

However, I have a new problem, which is that the addition of this function breaks Devel::PPPort on CentOS 5, which ships with Perl 5.8.8. See the linked bug report for the symptoms and the step-by-step on how I chased it down.

If you just want a simple test case, however:

$ sudo cpanm Mouse

Rolling back to 3.32 allows Mouse, Text::Xslate, and other Devel::PPPort-using modules to install via cpanm on this system.

mhx commented 8 years ago

I believe you're doing it wrong :)

The ppport.h generated by Devel::PPPort isn't meant to be generated automatically during the build process — which apparently is what you're doing — but rather meant to be updated occasionally and shipped with your code. The reason for that is just what you experienced: ppport.h can change, and these changes /may/ require modifications to your code. So you update on demand, because there's something you want to get out of using (a new version of) ppport.h.

In your case, your code is using gv_fetchpvs. Within ppport.h, this has a dependency on gv_fetchpvn_flags (which is the function you're having problems with). However, as providing gv_fetchpvn_flags produces real code, you have to explicitly enable support for it by defining NEED_gv_fetchpvn_flags.

If you run ppport.h, it actually tells you exactly that:

$ perl ppport.h xs-src/MouseTypeConstraints.xs
[...]
Uses gv_fetchpvs, which depends on gv_fetchpvn_flags, SVt_PVHV, Safefree, gv_fetchpv, savepvn
[...]
File needs gv_fetchpvn_flags, adding static request
[...]
--- xs-src/MouseTypeConstraints.xs
+++ xs-src/MouseTypeConstraints.xs.patched
[...]
+#define NEED_gv_fetchpvn_flags

So, you've got two options:

  1. Simply use the ppport.h generated by Devel::PPPort 3.32
  2. Use the latest ppport.h and define NEED_gv_fetchpvn_flags

In any case, you should stop generating ppport.h as part of the build process. This has never been and will likely never be supported.

jjn1056 commented 8 years ago

@mhx I have the exact same error with Data::Clone on an older CentoOS with Perl 5.8.8 and not sure I fully understand your explanation. FWIW downgrading also solves it, which is why it feels like a bug to me (given the point of this module I thought was to help with back compat, broken back compat feels icky). Are you saying this is a bug in Data::Clone, and that I should report there and the author is supposed to make a change like you described above?

I'm not actually using Data::Clone, its coming via HTML::FormHandler so this is impacting stuff well downstream dependency wise (and I'm seeing that if I look at modules that depend on Devel::PPPort, there's a growing trend of fails with older versions of Perl in basically everything that depends on this module. If all those module owners are 'holding it wrong' then we need to get the word out since this is having impact well downstream for those of us stuck on older perl.

You do actually have clear docs on this: https://metacpan.org/pod/Devel::PPPort but maybe those docs are more recent. When I look at Data::Clone I can't figure out if the author followed your instructions. I can see here https://metacpan.org/source/GFUJI/Data-Clone-0.004/inc/Module/Install/XSUtil.pm it looks like they are build requiring Devel::PPPort which I guess is not right but I have no idea what sort of patch to offer... suggestions?

wolfsage commented 8 years ago

"Uses gv_fetchpvs, which depends on gv_fetchpvn_flags, SVt_PVHV, Safefree, gv_fetchpv, savepv..."

So ppport.h knows which functions it provides and knows which ones depend on other functions. Can it implicitly set NEED_* in these cases without needing that to be added to consumers...?

mhx commented 7 years ago

@wolfsage, yes, adding NEED dependencies is something worth thinking about, but it's a completely different issue; a large part of the API doesn't even need any NEED definitions and having one for each API would bloat the module even further and simply defining all of them would bloat the code that's using D::PPP unnecessarily.

@jjn1056, build-requiring Devel::PPPort is just plain wrong. The generated ppport.h provides compatibilty for the module using it against a large number of Perl versions. However, upgrading ppport.h itself may require some work on the code using it, just as initially starting to use ppport.h does. Devel::PPPort was at no point meant to be compatible between releases, which is exactly why module authors are supposed to ship a particular version of ppport.h with their code.

atoomic commented 4 years ago

32 changed gv_fetchpvn_flags from a macro to a function

I'm going to check what is the status of Mouse 2.4.5 (and the last released version) and Data::Clone with an updated version of ppport.h to confirm the status of this issue

pali commented 4 years ago

And see also my (merged) PR "Fix gv_fetchpvn_flags": https://github.com/Dual-Life/Devel-PPPort/pull/85

atoomic commented 4 years ago

I can reproduce the described bug on CentOS 5 using Mouse v2.4.5 with the shipped ppport.h or updated ppport.h to 3.58.

This is fixed when using the last version available of Mouse v2.5.10 Updating ppport.h from Devel-PPPort at 3.58 for Mouse v2.5.10 is working fine

atoomic commented 4 years ago

Data-Clone-0.004 is using Module::Install::XSUtil helper use_ppport to dynamically generate ppport.h I checked that Data-Clone is currently working fine with an updated ppport.h from Devel-PPPort-3.58_01