Skip to content

Conversation

mohawk2
Copy link
Contributor

@mohawk2 mohawk2 commented Mar 20, 2021

Fix #16916

@jkeenan
Copy link
Contributor

jkeenan commented Mar 20, 2021

Fix #16916

The hesitation I have about applying this patch is that currently our test suite does not appear to exercise the "Error: No INPUT definition for type" message which your patch corrects. Consequently we have no way of demonstrating that the patch improves things.

I hacked up dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm --specifically generate_init() with print statements and ran the test suite. I was unable to locate any tests where the condition for entering the if block were met, i.e., $inputmap was always defined.

In your investigations preceding #16916 (comment), were you able to get the error-message to trigger? If so, could we re-work that into a unit test that we could use before applying the patch to illustrate the problem, then after applying to demonstrate the solution?

Thank you very much.
Jim Keenan

@mohawk2
Copy link
Contributor Author

mohawk2 commented Mar 20, 2021

@jkeenan I've now added a test, which hopefully helps.

An issue: 63b6fce introduces PERL_VERSION_LT, which doesn't exist on older Perls. The now-first commit here changes it to use PERL_VERSION_LE which already existed.

After that's resolved (perhaps by merging this), the first thing mentioned in #16916 still applies: the blead version of EU::PXS is 3.42. The CPAN version is 3.35, from 2017. @tsee @khwilliamson (and p5p) have permissions to upload. Could this be fixed?

mohawk2 added 2 commits March 20, 2021 17:56
As ExtUtils::ParseXS is dual-life it needs to use stable Perl macros.
@jkeenan jkeenan added the cpan-dual-life issues regarding dual-life cpan-first distributions label Mar 20, 2021
@jkeenan
Copy link
Contributor

jkeenan commented Mar 20, 2021

@jkeenan I've now added a test, which hopefully helps.

It does, indeed. I checked out your branch locally, temporarily reverted the change in ParseXS.pm, built perl and ran t/001-basic.t. It failed as expected. I then restored the change in ParseXS.pm, rebuilt and re-ran the test. It now passed, as intended.

(The test file uses a testing subroutine and module credited to me in 2010 that I had completely forgotten about. Perhaps that's because I got it from @xdg!)

An issue: 63b6fce introduces PERL_VERSION_LT, which doesn't exist on older Perls. The now-first commit here changes it to use PERL_VERSION_LE which already existed.

As to this internal variable, I'll hope that @tonycoz or @tsee takes a look at that, as that's outside my expertise. Otherwise, +1 from me.

After that's resolved (perhaps by merging this), the first thing mentioned in #16916 still applies: the blead version of EU::PXS is 3.42. The CPAN version is 3.35, from 2017. @tsee @khwilliamson (and p5p) have permissions to upload. Could this be fixed?

I have applied a 'cpan-dual-life' label to both this p.r. and to the original bug ticket. That may help.

Thank you very much.
Jim Keenan

@mohawk2
Copy link
Contributor Author

mohawk2 commented Mar 20, 2021

@jkeenan I've now added a test, which hopefully helps.

It does, indeed. I checked out your branch locally, temporarily reverted the change in ParseXS.pm, built perl and ran t/001-basic.t. It failed as expected. I then restored the change in ParseXS.pm, rebuilt and re-ran the test. It now passed, as intended.

As I checked (but am very pleased to have confirmed; more eyes = better). Thanks!

Fingers crossed those mentioned will be able to take a look soon enough.

@tonycoz
Copy link
Contributor

tonycoz commented Mar 24, 2021

It looks good to me.

@jkeenan jkeenan merged commit f1f3e12 into Perl:blead Mar 24, 2021
@mohawk2 mohawk2 deleted the pxs-bug branch March 25, 2021 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cpan-dual-life issues regarding dual-life cpan-first distributions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExtUtils::ParseXS release, plus bug with clang++ on 5.14-5.16 on EUPXS 3.04_01+

3 participants