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

Add missing prereq to dist.ini #12

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Sep 29, 2016

The module Win32::TieRegistry was found by
CPANTS to be
missing from the list of prerequisites for the module, although it is used.
Adding the prereq fixes the prereq_matches_use core kwalitee issue (as
tested via Test::Kwalitee::Extra).

This PR is intended to be helpful. If it needs to be changed in any way, please just let me know and I'll update it and resubmit.

Add missing prereq to dist.ini
The module `Win32::TieRegistry` was found by
[CPANTS](http://cpants.cpanauthors.org/release/SSIMMS/PDF-API2-2.028) to be
missing from the list of prerequisites for the module, although it is used.
Adding the prereq fixes the `prereq_matches_use` core kwalitee issue (as
tested via `Test::Kwalitee::Extra`).
@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

Win32::TieRegistry is only needed on Win32 platforms, so including it as a universal prerequisite doesn't seem like the right thing to do. Is there a way to limit it to that platform?

Owner

ssimms commented Oct 7, 2016

Win32::TieRegistry is only needed on Win32 platforms, so including it as a universal prerequisite doesn't seem like the right thing to do. Is there a way to limit it to that platform?

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 7, 2016

Contributor

You're quite right, that was daft of me... I'll see if I can limit the prerequisite and will resubmit the PR.

Contributor

paultcochrane commented Oct 7, 2016

You're quite right, that was daft of me... I'll see if I can limit the prerequisite and will resubmit the PR.

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 7, 2016

Contributor

The best bet seems to be to use the OSPrereqs plugin. It patches the Makefile.PL and adds Win32::TieRegistry to the list of prereqs if the OS is Win32. The downside is that CPANTS will still complain about the missing prerequisite. The upside is that Windows users will be able to install all dependencies without having to first run into the "oh, I need to install Win32::TieRegistry so that my code works" problem; it basically works transparently for them and the other platforms won't notice. What do you think? I can change my commit and do a force push so that you only have to merge the one commit.

Contributor

paultcochrane commented Oct 7, 2016

The best bet seems to be to use the OSPrereqs plugin. It patches the Makefile.PL and adds Win32::TieRegistry to the list of prereqs if the OS is Win32. The downside is that CPANTS will still complain about the missing prerequisite. The upside is that Windows users will be able to install all dependencies without having to first run into the "oh, I need to install Win32::TieRegistry so that my code works" problem; it basically works transparently for them and the other platforms won't notice. What do you think? I can change my commit and do a force push so that you only have to merge the one commit.

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

Ok, I think that makes this the same as pull request #1, which I just merged, so I'll close this one. Thanks for looking into it, and it'll be good to eliminate a potential stumbling block for Windows users.

Owner

ssimms commented Oct 7, 2016

Ok, I think that makes this the same as pull request #1, which I just merged, so I'll close this one. Thanks for looking into it, and it'll be good to eliminate a potential stumbling block for Windows users.

@ssimms ssimms closed this Oct 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment