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

fix building this module with current versions of Dist::Zilla and all #2

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dams

dams commented Feb 27, 2015

I've been assigned this module as part of the CPAN-PR challenge. I quickly noticed that the repo wouldn't build with current Dist::Zilla. This is mainly due to the fact that @rjbs changed PkgVersion plugin: now it doesn't wrap setting VERSION in a BEGIN block, making the XSLoader call at begin time fail.

I've fixed it in a Dist::Zilla PR: rjbs/Dist-Zilla#428 and used it in this PR (the trick is the use_begin option). Obviously you'll want to wait for the Dist::Zilla PR to be merged before merging this one.

Thank you !

@karenetheridge

This comment has been minimized.

karenetheridge commented on dist.ini in 4f19cf3 Feb 27, 2015

This is the wrong phase. Dist::Zilla plugins are only ever used on the authoring side, never for user installs.

This comment has been minimized.

Owner

dams replied Feb 27, 2015

indeed ! What is the right way to require a version at dzil build time then ? I've not found out how to specify just that. If you could give me an example, I'll add the PkgVersion version as well.

This comment has been minimized.

dolmen replied Feb 27, 2015

[Prereqs / DevelopRequires]
@karenetheridge

This comment has been minimized.

karenetheridge commented on dist.ini in 4f19cf3 Feb 27, 2015

If you are depending on an option recently added to a plugin, you need to use a :version = ... clause to indicate the minimum required version of that plugin.

This comment has been minimized.

Owner

dams replied Feb 27, 2015

Can you give me a more extended example ? I didn't find documentation about that

@karenetheridge

This comment has been minimized.

karenetheridge commented on dist.ini in 4f19cf3 Feb 27, 2015

There is no need to remove a plugin and then re-add it - just specify the option needed for that plugin right in the @filter specification.

This comment has been minimized.

Owner

dams replied Feb 27, 2015

I tried that but it didn't work:

[@Filter]
-bundle = @AVAR
option = for_basic
dist = re-engine-PCRE
use_MakeMaker    = 0
use_CompileTests = 0
bugtracker = rt
use_begin = 1

What would be the right way to specify the option ?

@karenetheridge

This comment has been minimized.

karenetheridge commented Feb 27, 2015

Why does XSLoader need to be run in a BEGIN block, anyway? I don't see anything else in lib/re/engine/PCRE.pm that depends on the loading to be happening this soon. Instead, you can remove the BEGIN block entirely, and then there is no issue with the version declaration.

@dams

This comment has been minimized.

dams commented Feb 27, 2015

Why does XSLoader need to be run in a BEGIN block, anyway? I don't see anything else in lib/re/engine/PCRE.pm that depends on the loading to be happening this soon. Instead, you can remove the BEGIN block entirely, and then there is no issue with the version declaration.

If I remove the BEGIN block entirely, numerous test fails. Granted, the tests might be fixed and worked around the loading at non-BEGIN time, but this situation might be not workaround-able, or maybe other modules actually require the begin block. To me, it's more elegant and more justified to provide the feature (that once were) to optionally enable the versioning to be in a BEGIN block, than change all modules that once depended on this. And I didn't start mentioning backward compatibility breakage :)

Anyway that's just my opinion, you obviously know more what should/shouldn't be done here.

@karenetheridge

This comment has been minimized.

karenetheridge commented Feb 27, 2015

What would be the right way to specify the option ?

I took a closer look at the @avar bundle, and indeed, since it doesn't use ConfigSlicer, it won't pick up and pass on any changes to the PkgVersion configs. So you're right - in this case, removing it and then re-adding it with the right options is all that can be done.

@karenetheridge

This comment has been minimized.

karenetheridge commented Feb 27, 2015

If I remove the BEGIN block entirely, numerous test fails. Granted, the tests might be fixed and worked around the loading at non-BEGIN time

That seems pretty fishy to me - and where I'd investigate first.

or maybe other modules actually require the begin block

I don't see how. use re::engine::PCRE; is an atomic operation as far as the caller is concerned, and after that statement finishes, everything in the module will have been loaded, whether or not it's in a BEGIN block.

This is where I'd ask @avar to weigh in and see if he can explain why the BEGIN block is there -- or if it's just a legacy holdover from when he adopted the module.

@dams

This comment has been minimized.

dams commented Feb 27, 2015

thanks @karenetheridge, I'll check with @avar. If you have some time, I'd be happy to see how to specify a plugin minimal version in dist.ini. Also, if you are against adding use_begin in PkgVersion, let me know early. To me, as I stated it, if I should bring up only one argument, it would be for backward compatibility.

@dolmen

This comment has been minimized.

dolmen commented Feb 27, 2015

Here is how to specifiy a minimum version for a plugin:

[Prereqs]
; Requires Dist::Zilla::Plugin::Prereqs 5.031
:version = 5.031

This works also with plugin bundles.

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