perl-pod / Pod-Spell

Pod::Spell perl module
https://metacpan.org/pod/Pod::Spell
Other
4 stars 8 forks source link

Does Pod::Spell need to depend on I18N::Langinfo #18

Closed nanis closed 9 years ago

nanis commented 9 years ago

This is based on the Stackoverflow question Install of Pod::Spell failing on Strawberry Portable 5.20.2. Even though I18N::Langinfo was not installed, all Pod::Spell tests passed, and podspell seemed to work.

I haven't looked at the tests or the code very carefully, but it seems I18N::Langinfo (which wouldn't work on Windows even if it were installed, is not required -- at least on Windows). Therefore, it might be best to remove it from the dependency list.

xenoterracide commented 9 years ago

the tests were non existent when I adopted the module, they have improved, but need work. I'm not really putting effort into maintaining perl stuff anymore but will apply patches if I receive them.

However since this dependency was added by @dolmen in the last release because he didn't deal with autoprereqs, it'd be really nice if he fixed it.

xenoterracide commented 9 years ago

oh, and it doesn't fail because the podspell command isn't tested at all, even by the compile tests I believe... @karenetheridge

nanis commented 9 years ago

Got it. As a data point, Perl::Critic declares a dependency on Pod::Spell, and builds and passes all its tests, at least on my system (without I18N::Langinfo). HTH.

Looking at @dolmen's commit, given the eval wrap around the use of I18N::Langinfo, it seems to me it is intended to use if it is available. Therefore, specifying it as a dependency doesn't seem appropriate to me.

Therefore, it seems to me that it is at best a candidate for recommends.

dolmen commented 9 years ago

@xenoterracide You're not just asking me to deal with AutoPrereqs. You're asking me to dig into your own personal dzil pluginbundle.

To every CPAN author that uses his/her own pluginbundle for CPAN modules: I'm just fed up with personal plugin blundles. This is much pain for contributors.

karenetheridge commented 9 years ago

@dolmen agreed, things like ensuring the prereqs are still correct is something the author should be verifying just before release. When dealing with a different person's config it's easy to miss things like this.

xenoterracide commented 9 years ago

On Sat, Mar 7, 2015 at 4:26 PM, Olivier Mengué notifications@github.com wrote:

You're not just asking me to deal with AutoPrereqs. You're asking me to dig into your own personal dzil pluginbundle.

To every CPAN author that uses his/her own pluginbundle for CPAN modules: I'm just fed up with personal plugin blundles. This is much pain for contributors

I don't see how I'm asking you to do that, sure AutoPrereqs is pulled in by my bundle, but that's as easy as filtering one in @Basic. Just filter it and then readd it to the dist.ini manually and have it filter and add the new deps as optional appropriately.

.. and yeah I hate them too but they are better than manually trying to manage them for every single dist, because so many plugins go bad, I only moved to one because of the sheer number of times I had to change plugins. So tired of having to deal with unmaintained or plugins that started failing or had bugs and no one fixing them. That's the only reason I created a bundle, so ever time someone broke a plugin I didn't have to update 20 modules dist.ini's.

The alternative of course is I revert your patch.

Caleb Cushing

http://xenoterracide.com

Calendar: https://www.google.com/calendar/embed?src=xenoterracide%40gmail.com&ctz=America/Chicago

karenetheridge commented 9 years ago

The alternative of course is I revert your patch.

That's pretty hostile. Why not just fix it, since you know what needs to be done? One has to dig into the bundle's code just to see if it's using AutoPrereqs, and then further to see if removing it would have other consequences.

If your bundle used the ConfigSlicer role it would be a lot easier:

[@Author::XENO]
Autoprereqs.skip = I18N::Langinfo
xenoterracide commented 9 years ago

On Sat, Mar 7, 2015 at 4:48 PM, Karen Etheridge notifications@github.com wrote:

That's pretty hostile. Why not just fix it, since you know what needs to be done? One has to dig into the bundle's code just to see if it's using AutoPrereqs, and then further to see if removing it would have other consequences.

If your bundle used the ConfigSlicer role it would be a lot easier:

I actually don't, I'm not sure what happens if you're on windows and using greater than 5.8.1 and you don't have it installed, does the patch work? or will podspell fail, are they all optional? I suspect this is a bigger problem for the patch than simply making the deps optional.

You may have to dig in to see if it's using AutoPrereqs, but running dzil test locally would have shown you that they were added.

Lastly I don't have the patience for fixing my pluginbundle. I am in maintenance mode only, my only concern is that my dists continue to work for people. I'm not going to invest energy into fixing other people's patches that add features.

Yeah if someone wants to request comaint and CC me, that's fine.

Caleb Cushing

http://xenoterracide.com

Calendar: https://www.google.com/calendar/embed?src=xenoterracide%40gmail.com&ctz=America/Chicago

karenetheridge commented 9 years ago

Yeah if someone wants to request comaint and CC me, that's fine.

ok, FWIW, I'm happy to take on maintainership of this dist. :)

karenetheridge commented 9 years ago

(feel free to give me comaint on everything/anything you want.. a few other authors have done that e.g. nothingmuch, sartak, perigrin so I can help out when new patches come in.)

xenoterracide commented 9 years ago

just so you know I'm not the primary maint on pause, I can't actually give you comaint, though I would suggest that the 2 of you both be added, and or make it a community module given its popularity.

xenoterracide commented 9 years ago

reverting I will reconsider readding the feature if someone ensures there is no breakage on windows.

A test case will be required

karenetheridge commented 9 years ago

podspell should be getting exercised by the compile tests - https://metacpan.org/source/XENO/Pod-Spell-1.17/t/00-compile.t#L16

And it looks like other MSWin32 systems had no trouble with the dist -- http://matrix.cpantesters.org/?dist=Pod-Spell%201.16

What is the problem with I18N::LangInfo? it's in core, since perl 5.7.3.

nanis commented 9 years ago

What is the problem with I18N::LangInfo? it's in core, since perl 5.7.3.

nl_langinfo doesn't exist on Windows, AFAIK. The module is supposed the croak when there is no support, but it doesn't actually seem to be installed at all on Windows (I haven't tried all Perl versions I can get my hands on).

cpanm croaks when a prerequisite does not exist. You can see from one of the successful tests on Strawberry that I18N::Langinfo is not installed.

 ! I18N::Langinfo                 0        n/a    `

Pod::Spell just started unconditionally requiring this module. I think @dolmen's patch is a good idea, but I do believe I18N::Langinfo should not be a hard requirement. You can see that @dolmen's patch already presumes that I18N::Langinfo is optional.

karenetheridge commented 6 years ago

As of 5.28, I18N::Langinfo will be available on all platforms. https://rt.perl.org/Public/Bug/Display.html?id=124032