cpan version conversion

[codecov[bot]]

Codecov Report

Merging #51 into master will decrease coverage by 0.01%. The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   90.64%   90.62%   -0.02%     
==========================================
  Files           1        1              
  Lines         171      192      +21     
  Branches       10       15       +5     
==========================================
+ Hits          155      174      +19     
  Misses         16       16              
- Partials        0        2       +2

Impacted Files	Coverage Δ
upt_macports/upt_macports.py	90.62% <90.47%> (-0.02%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 216634f...3815ccf. Read the comment docs.

[reneeotten]

The cpan_version function does the exact same conversion as MacPorts currently does, so comparing that to port info --version <port> for the "recursive packaging" and "update" using upt should work.

However, compared to the recommended-Perl-way of doing the conversion (using perl -Mversion -e 'print version->parse(*version*)->normal') there are some discrepancies that are probably worthwhile to investigate and perhaps correct both here as in the Perl PG procedure.

For example, versions that do not contain a . are not converted correctly compared to the Perl-way:

p5-perl-tidy    20190601 (portfile) ; 20190601 (port info --version) ; v20190601.0.0 (Perl) ; 20190601 (cpan_version function)

other ports in this category are: p5-test-regexp, p5-canary-stability, p5-x11-protocol-other, p5-uri-find, p5-test-perltidy, p5-regexp-common, p5-time-y2038, p5-mozilla-ca

@mojca @dbevans (or anybody else following this): does any of you remember if there was/is a reason for this?

Then there are some others that I have not checked yet carefully that do not give the same result compare to the Perl conversion, but do agree with port info --version: p5-nkf, p5-ifeffit, p5-business-isbn-data, p5-postscript-font, p5-mime-charset, p5-mime-encwords, p5-gearman

Some of these might just be mistakes in the Portfile or easy to fix unrelated problems so let's not worry about them you.... I'll look at some of those ports.

[Korusuke]

However, compared to the recommended-Perl-way of doing the conversion (using perl -Mversion -e 'print version->parse(*version*)->normal') there are some discrepancies that are probably worthwhile to investigate and perhaps correct both here as in the Perl PG procedure.

For example, versions that do not contain a . are not converted correctly compared to the Perl-way:

p5-perl-tidy    20190601 (portfile) ; 20190601 (port info --version) ; v20190601.0.0 (Perl) ; 20190601 (cpan_version function)

I looked into this a bit and from what I understand, perl conversion gives 'v20190601.0.0' but since there are no decimal points we don't need to worry about any issue of version comparison mismatch for which we are actually doing all this. So as per me we don't need to match everything to "perl conversion method", also because in macports v-string as version is not allowed as far as I know.

Also I guess since we are doing this for comparing versions we should cross check the conversion with: perl -Mversion -e 'print version->parse(*version*)->numify instead of perl -Mversion -e 'print version->parse(*version*)->normal

Doing this doesn't change @reneeotten analysis but the output changes a bit for most ports like for p5-perl-tidy now we have 20190601.000 instead of v20190601.0.0.

The perl conversion for p5-business-isbn-data is very weird:

using normal -> v20140910.2.999.999
using numify -> 20140910.002999999

[Korusuke]

Also I guess since we are doing this for comparing versions we should cross check the conversion with: perl -Mversion -e 'print version->parse(*version*)->numify instead of perl -Mversion -e 'print version->parse(*version*)->normal

I tried this out for all ports and I guess we should follow this only if we change it in portgroup as 'numify' changes things quite a lot for the ports where port --info version and perl normal were same.

[reneeotten]

@Korusuke I am not suggesting anywhere to change the conversion from "normal" (that is currently done) to "numify" in MacPorts. Chances for that to happen are probably not high anyway and I also don't see an obvious reason to do so. And, yes, of course port info will not give versions that start with a v, but I'm not sure that is observation is relevant here.

The only thing I pointed out is that the current MacPorts conversion isn't fully consistent with (the presumably intended) conversion of perl -Mversion -e 'print version->parse(*version*)->normal. That's -as you also pointed out- not an issue as long as we do the exact MacPorts conversion in also in the upt_macports backend. Eventually, if this conversion ends up in upt this is something to reconsider; of course, we can always just keep it the same and overwrite the needs_requirement functions as you've done in this PR.

So, there is no real reason to spend time on investigating this right now, let's instead fix/improve the conversion function itself and get the code in needs_requirement working correctly... or?

[jmroot]

The perl conversion for p5-business-isbn-data is very weird:

using normal -> v20140910.2.999.999
using numify -> 20140910.002999999

That would be a floating-point precision issue. (Which is one of the very good reasons not to use floating-point numbers for things that need to be compared exactly, especially when converted back and forth between binary and decimal representations.)

[Korusuke]

I guess we are ready to merge this? or are we waiting on upt-cpan to be updated? About tests for:

s = SpecifierSet(req.specifier)
            req.specifier = ','.join(
                [dep.operator + self.cpan_version(dep.version) for dep in s])

I tried searching for a way to test the value of variable req but to no luck. So if we want to really test it I guess we have to put that code in another function which return req, that way it can be tested. Is there any way I am missing that we can test the variable.

[reneeotten]

I guess we are ready to merge this? or are we waiting on upt-cpan to be updated?

I don't think so yet; there is still the comment on the cpan_version regarding why it has self as an argument (but doesn't seem to use it) and I asked you whether you tried to write that function in a more Pythonic way.

Also, what specific issue in upt-cpan would we be waiting for to be resolved (ping @Steap)? The issue you opened in that repository (https://framagit.org/upt/upt-cpan/issues/2) seems not relevant here. Or perhaps you're thinking about the code below from upt_cpan.py:

requirements[upt_phase] = [
    upt.PackageRequirement(module,
                           '' if spec == '0' else f'>={spec}')
    for (module, spec) in dependencies.get('requires', {}).items()
    if module != 'perl'  # This seems implicit
]

where it either seems that there is no specific version requirement ('' if spec == '0') or always assumes that the operator is >=? The latter one might indeed not be correct as we discussed earlier above. If that's what you're referring to that would be an issue/PR that belongs in that repository.

[Korusuke]

The latter one might indeed not be correct as we discussed earlier above. If that's what you're referring to that would be an issue/PR that belongs in that repository.

Yes I was referring to that, I am under the impression that @Steap is working on both the upt-cpan issues.

I asked you whether you tried to write that function in a more Pythonic way.

I changed it out a bit from the first version I had written but from the current state, I don't see any particular changes.

[reneeotten]

The latter one might indeed not be correct as we discussed earlier above. If that's what you're referring to that would be an issue/PR that belongs in that repository.

Yes I was referring to that, I am under the impression that @Steap is working on both the upt-cpan issues.

I opened an issue in upt-cpan for this.

[reneeotten]

@Korusuke @Steap I squashed the commits and made a few small modifications to the function that converts the CPAN version. If there are no further comments I intend to merge this on Saturday morning (EST time).