Bug #103833 for Parse-PMFile: What is the logic for flagging a dev release?

Mon Apr 20 05:11:40 2015 olaf [...] wundersolutions.com - Ticket created

Subject:

What is the logic for flagging a dev release?

Hi, Thanks for all of your work on this. :) I'm a bit unclear as to what constitutes a dev release. It appears that the only time that the version numbers are checked for underscores is at: https://metacpan.org/source/ISHIGAKI/Parse-PMFile-0.36/lib/Parse/PMFile.pm#L44 This condition is never checked if there's a valid META file containing provides. This means that if the META file contains a provides, it won't skip any versions with underscores, even if the provides actually does have the underscore. Also, I wonder if it should parse anything at all if the entire version is flagged as TRIAL, like in this case https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082899_14/META.yml#L71 I just ran up against this while trying to write some tests for OrePAN2. In some cases we want to index dev releases, but Parse::PMFile seems quite permissive, unless I misunderstand how it's supposed to work, which is entirely possible. ;) Olaf

Mon Apr 20 07:19:01 2015 ishigaki [...] cpan.org - Correspondence added

On Mon Apr 20 18:11:40 2015, OALDERS wrote: Show quoted text

> Hi, > > Thanks for all of your work on this. :) > > I'm a bit unclear as to what constitutes a dev release. > > It appears that the only time that the version numbers are checked for > underscores is at: > > https://metacpan.org/source/ISHIGAKI/Parse-PMFile- > 0.36/lib/Parse/PMFile.pm#L44 > > This condition is never checked if there's a valid META file > containing provides. This means that if the META file contains a > provides, it won't skip any versions with underscores, even if the > provides actually does have the underscore. > > Also, I wonder if it should parse anything at all if the entire > version is flagged as TRIAL, like in this case > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > 0.082899_14/META.yml#L71 > > I just ran up against this while trying to write some tests for > OrePAN2. In some cases we want to index dev releases, but > Parse::PMFile seems quite permissive, unless I misunderstand how it's > supposed to work, which is entirely possible. ;) > > Olaf

Hi. As the name (::PMFile) suggests, this module is to parse and extract a VERSION in a specific .pm file. So, you can use this to look for versions in a distribution, but this doesn't know (and care) anything about a version of a distribution by itself -- namely, it doesn't care if a distribution has -TRIAL in its name, and it doesn't try to parse a .pm file if a given META file has a provides field (it just trusts you've done the right thing). Also, it doesn't respect file/directory fields in no_index in META though it does respect package/namespace fields (because the formers are for a distribution, and the latters are relevant to each .pm file). You might also want to look at Parse::LocalDistribution. Though it's not for an archived distribution (so it doesn't still care about -TRIAL), it does treat versions in an extracted distribution more correctly -- or more predictable. Hope this helps.

Mon Apr 20 07:19:02 2015 The RT System itself - Status changed from 'new' to 'open'

Tue Apr 21 11:51:31 2015 ether [...] cpan.org - Correspondence added

Just FYI -- I plan to add an is_trial interface to Module::Metadata "at some point" -- at least before MMD is added to PAUSE's code. It will understand _ versions as well as detect the common # TRIAL comment that appears in distributions that just set release_status in meta and use -TRIAL tarball names (e.g. Dist::Zilla-based dists).

Wed Apr 22 21:29:50 2015 olaf [...] wundersolutions.com - Correspondence added

On Mon Apr 20 07:19:01 2015, ISHIGAKI wrote: Show quoted text

> On Mon Apr 20 18:11:40 2015, OALDERS wrote:

> > Hi, > > > > Thanks for all of your work on this. :) > > > > I'm a bit unclear as to what constitutes a dev release. > > > > It appears that the only time that the version numbers are checked > > for > > underscores is at: > > > > https://metacpan.org/source/ISHIGAKI/Parse-PMFile- > > 0.36/lib/Parse/PMFile.pm#L44 > > > > This condition is never checked if there's a valid META file > > containing provides. This means that if the META file contains a > > provides, it won't skip any versions with underscores, even if the > > provides actually does have the underscore. > > > > Also, I wonder if it should parse anything at all if the entire > > version is flagged as TRIAL, like in this case > > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > > 0.082899_14/META.yml#L71 > > > > I just ran up against this while trying to write some tests for > > OrePAN2. In some cases we want to index dev releases, but > > Parse::PMFile seems quite permissive, unless I misunderstand how it's > > supposed to work, which is entirely possible. ;) > > > > Olaf

> > Hi. > > As the name (::PMFile) suggests, this module is to parse and extract a > VERSION in a specific .pm file. So, you can use this to look for > versions in a distribution, but this doesn't know (and care) anything > about a version of a distribution by itself -- namely, it doesn't care > if a distribution has -TRIAL in its name, and it doesn't try to parse > a .pm file if a given META file has a provides field (it just trusts > you've done the right thing). Also, it doesn't respect file/directory > fields in no_index in META though it does respect package/namespace > fields (because the formers are for a distribution, and the latters > are relevant to each .pm file). > > You might also want to look at Parse::LocalDistribution. Though it's > not for an archived distribution (so it doesn't still care about > -TRIAL), it does treat versions in an extracted distribution more > correctly -- or more predictable. > > Hope this helps.

Thanks for this. My use case is actually OrePAN2 which is using Parse::LocalDistribution. (Let's just take the -TRIAL bit off the table -- that's likely muddying the waters here). The problem (as I see it) is the second case I've described in this comment: https://github.com/tokuhirom/OrePAN2/pull/36#issuecomment-95380071 If I index a file using Parse::LocalDistribution and don't explicitly allow dev releases, in the case of DBIx::Class I get a list of modules -- but just not DBIx::Class itself. So, it seems to think that just the top level namespace contains the developer release. The behaviour itself seems confusing to me. I think we're going to work around this in OrePAN2 (hopefully as I've proposed). Probably this could become somewhat clearer by having 1 standard way of declaring a dev release, but as it stands I'm not sure how helpful the dev checking is in Parse::PMFile. If it's checking for underscores in module versions, it could also do that when getting the names and versions via provides. Anyway, I'm not actually asking for a fix here because we'll work around it, but I at least wanted to point out that I think there are cases which aren't being caught. :) BTW, I hope you had a good flight home from Berlin!

Wed Apr 22 21:30:19 2015 olaf [...] wundersolutions.com - Correspondence added

On Tue Apr 21 11:51:31 2015, ETHER wrote: Show quoted text

> Just FYI -- I plan to add an is_trial interface to Module::Metadata > "at some point" -- at least before MMD is added to PAUSE's code. It > will understand _ versions as well as detect the common # TRIAL > comment that appears in distributions that just set release_status in > meta and use -TRIAL tarball names (e.g. Dist::Zilla-based dists).

Thanks, ETHER. That will be a helpful addition. :)

Thu Apr 23 01:41:12 2015 ishigaki [...] cpan.org - Correspondence added

On Thu Apr 23 10:29:50 2015, OALDERS wrote: Show quoted text

> On Mon Apr 20 07:19:01 2015, ISHIGAKI wrote:

> > On Mon Apr 20 18:11:40 2015, OALDERS wrote:

> > > Hi, > > > > > > Thanks for all of your work on this. :) > > > > > > I'm a bit unclear as to what constitutes a dev release. > > > > > > It appears that the only time that the version numbers are checked > > > for > > > underscores is at: > > > > > > https://metacpan.org/source/ISHIGAKI/Parse-PMFile- > > > 0.36/lib/Parse/PMFile.pm#L44 > > > > > > This condition is never checked if there's a valid META file > > > containing provides. This means that if the META file contains a > > > provides, it won't skip any versions with underscores, even if the > > > provides actually does have the underscore. > > > > > > Also, I wonder if it should parse anything at all if the entire > > > version is flagged as TRIAL, like in this case > > > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > > > 0.082899_14/META.yml#L71 > > > > > > I just ran up against this while trying to write some tests for > > > OrePAN2. In some cases we want to index dev releases, but > > > Parse::PMFile seems quite permissive, unless I misunderstand how > > > it's > > > supposed to work, which is entirely possible. ;) > > > > > > Olaf

> > > > Hi. > > > > As the name (::PMFile) suggests, this module is to parse and extract > > a > > VERSION in a specific .pm file. So, you can use this to look for > > versions in a distribution, but this doesn't know (and care) anything > > about a version of a distribution by itself -- namely, it doesn't > > care > > if a distribution has -TRIAL in its name, and it doesn't try to parse > > a .pm file if a given META file has a provides field (it just trusts > > you've done the right thing). Also, it doesn't respect file/directory > > fields in no_index in META though it does respect package/namespace > > fields (because the formers are for a distribution, and the latters > > are relevant to each .pm file). > > > > You might also want to look at Parse::LocalDistribution. Though it's > > not for an archived distribution (so it doesn't still care about > > -TRIAL), it does treat versions in an extracted distribution more > > correctly -- or more predictable. > > > > Hope this helps.

> > Thanks for this. My use case is actually OrePAN2 which is using > Parse::LocalDistribution. (Let's just take the -TRIAL bit off the > table -- that's likely muddying the waters here). > > The problem (as I see it) is the second case I've described in this > comment: > > https://github.com/tokuhirom/OrePAN2/pull/36#issuecomment-95380071 > > If I index a file using Parse::LocalDistribution and don't explicitly > allow dev releases, in the case of DBIx::Class I get a list of modules > -- but just not DBIx::Class itself. So, it seems to think that just > the top level namespace contains the developer release. > > The behaviour itself seems confusing to me. I think we're going to > work around this in OrePAN2 (hopefully as I've proposed). Probably > this could become somewhat clearer by having 1 standard way of > declaring a dev release, but as it stands I'm not sure how helpful the > dev checking is in Parse::PMFile. If it's checking for underscores in > module versions, it could also do that when getting the names and > versions via provides.

Well, I'm not sure what actually confuses you, but let's see the code of PAUSE a bit. First of all, PAUSE ignores developer releases and just skips without parsing anything in those releases. https://github.com/andk/pause/blob/master/lib/PAUSE/dist.pm#L296 https://github.com/andk/pause/blob/master/lib/PAUSE/mldistwatch.pm#L416 In other words, PAUSE assumes a distribution it actually parses is not a dev release. (So do Parse::PMFile/LocalDistribution, unless you explicitly specify otherwise. If this is what confused you, or what you considered not covered, I should improve docs in Parse::PMFile/LocalDistribution and/or OrePAN.) Now let's consider PAUSE starts processing. It looks for a valid META data, and if it finds that, it trusts that data without further parsing. Otherwise, it tries to parse VERSIONs in files, and while parsing, it also ignores packages with a version that contains an underscore (in a non-dev distribution, to get rid of dev versions from PAUSE indices entirely). https://github.com/andk/pause/blob/master/lib/PAUSE/pmfile.pm#L279 In the case of DBIx-Class-0.082899_14 (which doesn't have a "provides" field in its META files), if PAUSE didn't have the first dist version check (or if you just pass that distribution to Parse::LocalDistribution without allowing dev versions), it would actually parse files and ignore the DBIx::Class package because its version contains an underscore (not because it's a main module in the distribution which in fact is a developer release, etc), and also it would index SQL::Translator stuff because they have valid versions and PAUSE (and Parse::LocalDistribution) doesn't care if a package in a distribution has the same namespace as the main namespace or not, as long as the namespace is valid and the uploader has a proper permission for that. https://github.com/andk/pause/blob/master/lib/PAUSE/pmfile.pm#L242 That being said, there are cases when we want to do something different from what PAUSE does. Thus Parse::PMFile/LocalDistribution have an option not to ignore dev versions. To summarize, 1) Pass a non-dev local distribution to Parse::LocalDistribution without allowing dev release -- ok, that should be (almost) the same as what PAUSE does. 2) Pass a non-dev local distribution to Parse::LocalDistribution with allowing dev release -- probably ok, though if the distribution has a pmfile that has a underscored version (which should be rare), it's also listed/indexed contrary to what PAUSE does. 3) Pass a dev local distribution to Parse::LocalDistribution without allowing dev release -- bad assumption, confusing results where the main package (and possibly other packages with the same version) are not listed/indexed. 4) Pass a dev local distribution to Parse::LocalDistribution with allowing dev release -- probably ok, everything should be listed/indexed including dev versions. Have I made myself clear? Show quoted text

> > Anyway, I'm not actually asking for a fix here because we'll work > around it, but I at least wanted to point out that I think there are > cases which aren't being caught. :) > > BTW, I hope you had a good flight home from Berlin!

BTW. Thanks, the flight was pretty good :)