Review: extract versions more safely

[karenetheridge]

• use code pioneered in https://github.com/dagolden/Version-Eval and updated by @haarg to extract versions more safely from .pm files
• there originally was a dependency on 5.008001; I removed this, but I am not sure if it is actually needed

[karenetheridge]

(there are also some recent commits in master that could be looked at, although they should all be uncontroversial.)

[haarg]

Passes its t/ tests on 5.6.2

[dagolden]

Other than the one or two comments I made, I think it's reasonable, though it should stay -TRIAL for a while, I'm pretty sure.

Side note: Andreas actually runs his separate process as another, restricted user as well. I don't think we need that, because a restricted user could be made to run M::M itself.

[karenetheridge]

I'm going to rebase this branch against the new master so it can be merged at QAH 2015 - I'm embarrassed that I've let this sit a whole year.

[karenetheridge]

rebased to v1.000020 so far (a pristine copy of the PR is pushed to topic/safe_versions_orig)

[karenetheridge]

I've redone this PR on top of the new master, but tests are failing because the work failed to account for an API requirement that didn't have tests last year but is now being tested -- that the returned version must always be a version.pm object. Therefore we need another wrapper layer to sanitize the version returned by eval_version(). I'm working on that now.

[haarg]

I'm pretty sure that not returning a version object was am intentional part of the changes here. I think we at least want to preserve that behavior under a different name.

[karenetheridge]

@haarg the path that I'm assuming presently is that Module::Metadata::ExtractVersion::eval_version should return, in string form, whatever it finds, but that Module::Metadata->new(...)->version(...) should continue to return a version object. It may make sense to create new internal hash keys to store the original stringy form, as they may not convert to a version object without further sanitization.

[karenetheridge]

...however, we may want to switch the public API to returning strings?... which would need more discussion as well as thorough testing :)

[haarg]

Generally I think it makes sense for the API to return the same form that exists in the file, which could be a string or a version object.

[dagolden]

I think that depends on the purpose. For display, a string is fine. For
analysis or comparison or metadata, only a lax version string is useful so
it can become a version object for comparison or normalization.
On Apr 13, 2015 4:57 PM, "Graham Knop" notifications@github.com wrote:

Generally I think it makes sense for the API to return the same form that
exists in the file, which could be a string or a version object.

—
Reply to this email directly or view it on GitHub
#1 (comment)
.

[karenetheridge]

Force-pushed with some cleanup commits inserted prior to these changes, which extract the raw version string separately and then inflate it to a version object, as we were doing before.

Now there is just one test failure (my debugging output included):

# using dist Simple47 in /tmp/MMD-_WnoG6xm/Simple47
# creating /tmp/MMD-_WnoG6xm/Simple47/lib/Simple.pm
### B: got raw vers my $version = atan2(1,1) * 4; $Simple::VERSION = "$version"; for package, Simple, line my $version = atan2(1,1) * 4; $Simple::VERSION = "$version";
### -> got a raw vers: my $version = atan2(1,1) * 4; $Simple::VERSION = "$version"; from line my $version = atan2(1,1) * 4; $Simple::VERSION = "$version";
### -> got a raw vers: my $version = atan2(1,1) * 4; $Simple::VERSION = "$version"; from line 1;
##### for raw version string: my $version = atan2(1,1) * 4; $Simple::VERSION = "$version";
##### _dwim_version upgraded that to 0
ok 144 - An object of class 'version' isa 'version'
not ok 145 - case 47: module version passes match sub
#   Failed test 'case 47: module version passes match sub'
#   at t/metadata.t line 450.
ok 146 - case 47: no warnings from parsing
# $VAR1 = {
#           'got' => bless( {
#                             'version' => [
#                                            0
#                                          ],
#                             'original' => '0'
#                           }, 'version' ),
#           'module_contents' => 'package Simple;
# my $version = atan2(1,1) * 4; $Simple::VERSION = "$version";
# 1;
# '
#         };

In master, the test looks like this:

# using dist Simple46 in /tmp/MMD-ujhwrC_e/Simple46
# creating /tmp/MMD-ujhwrC_e/Simple46/lib/Simple.pm
### B: got raw vers ??? from Simple
##### _evaluate_version_line found 3.14159265358979
##### _evaluate_version_line upgraded that to 3.14159265358979
### B: which is upgraded to 3.14159265358979

[karenetheridge]

This test does not pass because the operation is not permitted: ERROR: 'atan2' trapped by operation mask at (eval 7) line 4.

However I'm going to have to back up again and write tests for both the raw version as well as the final (version.pm-inflated) version, to be sure we aren't regressing there in relation to master. FML :(