CPAN::Meta::Merge + merging "release_status" is undesirable.

[kentfredric]

As seen on #[email protected]

This code block involves CPAN::Meta::Merge, and renders some painful problems, and its not clear if its a dzil bug, or a CPAN::Meta::Merge bug or both (that is to say, its not obvious where code has to be changed for this to continue to work)

sub _build_distmeta {
  my ($self) = @_;

  require CPAN::Meta::Merge;
  my $meta_merge = CPAN::Meta::Merge->new(default_version => 2);
  my $meta = {
...
    release_status => ($self->is_trial or $self->version =~ /_/)
                    ? 'testing'
                    : 'stable',
...
  };

  for ($self->plugins_with(-MetaProvider)->flatten) {
    $meta = $meta_merge->merge($meta, $_->metadata);
  }

  return $meta;
}

Now, you combine this block of code with a plugin that does

[Metadata]
release_status = unstable

And the side effect is as if they'd done as follows:

use CPAN::Meta::Merge;
my $meta_merge = CPAN::Meta::Merge->new(default_version => 2);
my $meta = { 'release_status' => 'stable' };
$meta = $meta_merge->merge( $meta, { 'release_status'  => 'unstable' } );
# Can't merge attribute release_status

Interestingly, replacing the second value with a bogus value like "development" or "asdf" silently has no problem here

use CPAN::Meta::Merge;
my $meta_merge = CPAN::Meta::Merge->new(default_version => 2);
my $meta = { 'release_status' => 'stable' };
$meta = $meta_merge->merge( $meta, { 'release_status'  => 'Beware the jabberwock my son' } );

( I suspect due to the invalid term simply getting ignored and aliased to "stable" )

use CPAN::Meta::Merge;
my $meta_merge = CPAN::Meta::Merge->new(default_version => 2);
my $meta = { 'release_status' => 'testing' };
$meta = $meta_merge->merge( $meta, { 'release_status'  => 'BEWARE THE JABBERWHAT!?' } );
# Can't merge attribute release_status at

More over, the failure message it renders in such a case could be improved either way, had to do a bit of debugging to trace what was causing it =)

Can't merge attribute release_status at <some line in Dist::Zilla>

Could be more descriptive + more context.

[Leont]

This code block involves CPAN::Meta::Merge, and renders some painful problems, and its not clear if its a dzil bug, or a CPAN::Meta::Merge bug or both (that is to say, its not obvious where code has to be changed for this to continue to work)

I don't think the current default is wrong, but one could argue another behavior is better. I'm not sure what's the best way to approach this. Possibly adding a prefer-right merger.

Interestingly, replacing the second value with a bogus value like "development" or "asdf" silently has no problem here

This is probably because CPAN::Meta::Convert getting in between. I'm increasingly feeling like that was a bad idea.

More over, the failure message it renders in such a case could be improved either way, had to do a bit of debugging to trace what was causing it =)

Already done ;-)

[dagolden]

Semantically, I think merging a more stable status with a less stable status should result in the less stable status. I.e. you can downgrade with a merge, but you can't upgrade. I can't think of a good use case for going the other way. I think that would "fix" the dzil example.

[karenetheridge]

09:36 < ether> I'm going to put out there my initial supposition: when each plugin supplies metadata, it should also now supply a hints hash where it indicates the merge policy that should be in effect (or let dzil decide, if omitted): "this should override whatever's there", "try to merge sanely and die if there ...
09:36 < ether> ... is a conflict", or "let the wookie win if he got there first"
09:36 < ether> with the default being "try to reconcile everything, and die if you cannot"
09:37 * kentnl was thinking there would be a "+" prefix on a key for "clobber this if it exists"
09:37 < ether> since dzil itself supplies the release status field, other plugins still should have a way to alter that
09:37 < ether> right now they cannot because two scalar fields of different values cannot be merged

[karenetheridge]

https://rt.cpan.org/Ticket/Display.html?id=99852 holds another case where merging falls down flat. I'm still poking at this to see why a 'dzil build' in my bundle repo works fine, but the tests cause merging to blow up.

[Leont]

Semantically, I think merging a more stable status with a less stable status should result in the less stable status. I.e. you can downgrade with a merge, but you can't upgrade. I can't think of a good use case for going the other way. I think that would "fix" the dzil example.

Sounds reasonable, and shouldn't be too hard to implement.

[van-de-bugger]

Hi, I am not sure if my problem is the problem you are discussing. If not, I'll open a new issue. In my Makefile.PL I have following code:

WriteMakefile( VERSION => '0.01', ..., META_MERGE => { release_status => 'unstable' }, ... );

However, in META.json I always get

"release_status" : "stable",

I tried replacing META_MERGE with META_ADD with no success — in META.json release_status is always stable. However, it depends on VERSION — if version contains underscore, release_status becomes testing. So, release_status always computed automatically and I have no control over it.

I tracked the problem down to CPAN::Meta::Converter::convert (v2.150001). This sub gets release_statatus as specified in Makefile.PL, and returns it "converted".

[dagolden]

@van-de-bugger are you using Dist::Zilla? If so, it forces release_status based on version. There's a pull request to change that, not yet merged: rjbs/Dist-Zilla#438

[van-de-bugger]

are you using Dist::Zilla?

Not yet. In that experiment I used hand-made plain Makefile.PL.

[dagolden]

Not yet. In that experiment I used hand-made plain Makefile.PL.

Is this in a repo somewhere? I'd like to take a look. CPAN::Meta::Converter is supposed to respect an existing release_status.

[van-de-bugger]

Here is a simple test case:

$ cat test.pl 
use strict;
use warnings;
use CPAN::Meta;
use Data::Dumper;

print( "$CPAN::Meta::VERSION\n" );

my $attrs = {
    name => 'Foo',
    version => '0.01',
    release_status => 'unstable',
};
print( "\$attrs =\n", Dumper( $attrs ) );
my $meta = CPAN::Meta->new( $attrs );
print( "\$meta =\n", Dumper( $meta ) );

$ perl test.pl 
2.150001
$attrs =
$VAR1 = {
          'name' => 'Foo',
          'release_status' => 'unstable',
          'version' => '0.01'
        };
$meta =
$VAR1 = bless( {
                 'version' => '0.01',
                 'release_status' => 'stable',
                 'generated_by' => 'CPAN::Meta::Converter version 2.150001',
                 'name' => 'Foo',
                 'dynamic_config' => 1,
                 'author' => [
                               'unknown'
                             ],
                 'license' => [
                                'unknown'
                              ],
                 'meta-spec' => {
                                  'version' => 2,
                                  'url' => 'http://search.cpan.org/perldoc?CPAN::Meta::Spec'
                                },
                 'prereqs' => {},
                 'abstract' => 'unknown'
               }, 'CPAN::Meta' );

[dagolden]

That sure seems like a bug. I'll dig into it.

[mohawk2]

@van-de-bugger, recently you've reported a number of bugs, which is great. The next stage of greatness is if your test could be in the form of a *.t file which uses eg Test::More, which you could run with prove, and which authors could copy/paste straight into a new test, making things permanent going forward.

[dagolden]

@van-de-bugger, fixed in 15850b3. The "problem" was that release_status wasn't in older spec version and the 1.x to 2.0 upconversion ignored it. It doesn't hurt to keep it if it exists.