Skip to content

Perl 7, XS and Devel::PPPort

Jump to bottom
Karen Etheridge edited this page Jul 21, 2020 · 58 revisions

Perl 7, XS and Devel::PPPort

☛ feel free to edit, comment, react on this documment

Introduction

As proposed here, The Perl major version will soon be bumped from 5 to 7, opening the road to 8, 9, ... later on.

The Problems

When bumping the Perl Major version to 7, this is going to raise two different but related issues with XS code.

  1. Incorrect and common usage of PERL_VERSION checks in xs code
  2. An existing protection in ppport.h provided by Devel::PPPort to die if the major version is not 5

Incorrect usage of PERL_VERSION

Description of the problem

It is a common pattern(even if incorrect) to check only PERL_VERSION in xs code:

#if PERL_VERSION < 13 || (PERL_VERSION == 13 && PERL_SUBVERSION < 9)
#  define PERL_HAS_BAD_MULTICALL_REFCOUNT
#endif

read more from cpan/Scalar-List-Utils/ListUtil.xs

The check is incomplete. It is missing a check on PERL_REVISION. This code assumes Perl will never exceed version 5.

Exploring solutions

Using PERL_REVISION

An improved usage of PERL_VERSION would always check the PERL_REVISION

#if PERL_REVISION == 5 && ( PERL_VERSION < 13 || (PERL_VERSION == 13 && PERL_SUBVERSION < 9) )
#  define PERL_HAS_BAD_MULTICALL_REFCOUNT
#endif

Using a macro to perform the comparison

A much better solution would be to use some alternate macro designed for the job. If you look at existing XS, these macros can be seen copied in multiple XS files.

Here is a version of them using multiple flavors depending what check you need

#ifndef PERL_VERSION_DECIMAL
#  define PERL_VERSION_DECIMAL(r,v,s) (r*1000000 + v*1000 + s)
#endif
#ifndef PERL_DECIMAL_VERSION
#  define PERL_DECIMAL_VERSION \
        PERL_VERSION_DECIMAL(PERL_REVISION,PERL_VERSION,PERL_SUBVERSION)
#endif
#ifndef PERL_VERSION_GE
#  define PERL_VERSION_GE(r,v,s) \
        (PERL_DECIMAL_VERSION >= PERL_VERSION_DECIMAL(r,v,s))
#endif
#ifndef PERL_VERSION_LE
#  define PERL_VERSION_LE(r,v,s) \
        (PERL_DECIMAL_VERSION <= PERL_VERSION_DECIMAL(r,v,s))
#endif
#ifndef PERL_VERSION_GE
#  define PERL_VERSION_GT(r,v,s) \
        (PERL_DECIMAL_VERSION > PERL_VERSION_DECIMAL(r,v,s))
#endif
#ifndef PERL_VERSION_LE
#  define PERL_VERSION_LT(r,v,s) \
        (PERL_DECIMAL_VERSION < PERL_VERSION_DECIMAL(r,v,s))
#endif
#  define PERL_VERSION_EQ(r,v,s) \
        (PERL_DECIMAL_VERSION == PERL_VERSION_DECIMAL(r,v,s))
#endif
...

The check above could then become

#if PERL_VERSION_LT(5,13,9)
#  define PERL_HAS_BAD_MULTICALL_REFCOUNT
#endif

Note that IMO, this is cleaner, easier to read and probably reduces mistakes...

Impact on CPAN

Protections in ppport.h

The second issue we are facing comes from an internal protection in ppport.h itself which breaks if PERL_REVISION is not 5. Over 1017 XS modules on CPAN include a ppport.h that needs to be updated and a re-released to CPAN.

For example Class-XSAccessor-1.19 comes with ppport.h This has been fixed by Karl via this commit.

The main issue was coming from the check in parts/inc/version

/* It is very unlikely that anyone will try to use this with Perl 6
   (or greater), but who knows.
 */
#if PERL_REVISION != 5
#  error ppport.h only works with Perl version 5
#endif /* PERL_REVISION != 5 */

Questions / Exploring Solutions

  1. Why does ppport.h need to be shipped with each CPAN distro?
  2. Devel-PPPort could start shipping ppport.h in a shared_lib directory which could then be consumed by other modules
  3. Should toolchain patch CPAN installs automatically update ppport.h when detecting a dated copy of the header file?
    • Is patching all of CPAN the only way?
  4. What other solutions can we consider to try to solve that problem?

Incomplete PERL_REVISION checks

Many XS modules (152 on CPAN) are using #if PERL_VERSION Most do not check for PERL_REVISION correctly.

There are several ways we have thought of already to fix this problem. There may be more:

  1. Provide the compare macros (PERL_VERSION_GT(5,21,2) via Devel::PPPort - view work from Karl
  2. Patch Perl core itself rather than patching all modules

Compare macros

Karl has already worked on providing these macros:

These macros will also be added to CORE in the next development cycle.

I think we should consider patching all, if not most, of popular modules (high in the CPAN river or dual life) to enforce the usage of these new macros.

We should also add a documentation section for XS to recommend using these macros.

Conflicting redefined macros may be a problem. Example: version.pm defines PERL_VERSION_LT. At first glance, it looks like this will only result in a compiler warning which might actually be a good thing!

This solution only fixes the issue if authors update the ppport.h they ship with their module.

I think adding the compare macros to Devel::PPPort is a good thing and should be done and released.

Patch Perl 7

Patching CPAN is hard. We are still working on fixing modules on CPAN which assume . is in @INC. 3 years later, 5000 modules on CPAN make incorrect use of Module::Install and are broken for the basic use case of perl Makefile.PL; make install

The following approach would allow any existing XS code to warn but continue to work as it did previously in Perl 7 and beyond.

  1. Make these constants forever stuck at:
    • PERL_REVISION = 5
    • PERL_VERSION = 100 # Something high.
    • PERL_SUBVERSION= 0.
  2. Deprecate and warn on any usage of these variables.
    • gcc has a feature to warn on use of deprecated variables.
  3. Replace these constants in core with something else: PERL_VERSION_MAJOR, PERL_VERSION_MINOR, PERL_VERSION_RELEASE
    • Discourage their use outside of core.
  4. Encourage the usage of PERL_VERSION_GT macros instead.
  5. Patch Devel::PPPort to use PERL_VERSION_LE macros instead of VERSION checks on perl 7 and above

If we choose this plan, I would also like to discuss how the #define will work for PERL_VERSION_MAJOR. I think they should be inline functions which extract from a single source of knowledge (PERL_CORE_VERSION=7.3.8) instead of integers.

ppport.h would do something like this:

#ifndef PERL_VERSION_DEPRECATED <--- we're on 5
# define GT macros.
#else
... Do nothing
#endif

From khw:

I'll cut to the bottom line. It's easy, using an idea from Todd, to change core and ppport.h going forward so that no module need change existing source to get proper version control, and adding the macros allows future code to do it right.

But the problem is that this requires a new version of ppport.h. When a new version of a module is released, the author is supposed to drop in the latest ppport.h, but not all do, and we want existing module releases to work with new perls if they aren't doing the things we are trying to get rid of. It appears to several of this that the code that installs a module onto a perl instance needs to know about and use the latest ppport.h. And that is really what this working group should be looking at.

I believe Paul has some working solutions to similar problems. I'd like to hear from anyone with ideas

From tonyc:

I think the best we can do is update Devel::PPPort to use the version comparison macros, and document them as best practice.

If we decide that it's preferable that dists use Devel::PPPort as a dependency and not bundle then document that both in Devel::PPPort and perlxs, with examples.

As to the version macros I'm not entirely comfortable with making the old PERL_REVISION macros lie, but I don't strongly object to it if it's going to keep backward compatibility that well. Will $] also return 5.999999?

From jkeenan:

A side note: As of this weekend, in the p7 branch in @atoomic's repository, all tests for dist/Devel-PPPort/t/*.t now PASS and run without warnings.

From ether:

If the module declares itself as v7 compatible, then the old PERL_REVISION macros should not be available at all. (As stated, the old macros were bad because you could test minor version independently of major version, so people got lazy and omitted major version - this behaviour should no longer be allowed.) Instead, an updated-for-p7 module should use the new proposed macro where both major and minor versions must be passed.

We could also add a deprecation warning during compilation when the old macros are used (so any module author updating to a newer ppport.h will see it) advising to switch to the new macros.

I would also add a check to all ppport.h versions starting today, advising that the author upgrade if the file is over N days old (with N to be bikeshed but a value of something like 60 to 90 days might be reasonable, as Devel::PPPort sees frequent releases). This requires embedding the release timestamp into the file, so it won't work for any existing ppport.h files in the wild of course. Or, alternatively, the age of the file could be inferred from the latest blead version that the file supports. but since changes need to be made anyway to add these checks, we might as well be direct and use a timestamp.

(Note: a common trick used in toolchain to do author-only warnings, so users installing from cpan do not see the same warnings, is -d '.git' or !-f 'META.json'.)

https://metacpan.org/release/perl

Clone this wiki locally