miyagawa / Carmel

CPAN Artifact Repository Manager
Other
138 stars 17 forks source link

Issue with modules generated by .pm.PL file without META (e.g. Proc::PID::File::Fnctl) #71

Closed Grinnz closed 2 years ago

Grinnz commented 2 years ago

With the cpanfile: requires 'Proc::PID::File::Fcntl';

carmel install fails consistently:

> carmel install
---> Installing modules...
---> Installing new dependencies: Proc::PID::File::Fcntl
Successfully installed Proc-PID-File-Fcntl-1.01
1 distribution installed
Can't find an artifact for Proc::PID::File::Fcntl => 1.01
You need to run `carmel install` first to get the modules installed and artifacts built.
miyagawa commented 2 years ago

I can't reproduce here. Which version of Carmel and perl do you use?

➜  cat cpanfile
requires 'Proc::PID::File::Fcntl';

➜  carmel
---> Installing new dependencies: Proc::PID::File::Fcntl
Successfully installed Proc-PID-File-Fcntl-1.01
1 distribution installed
Using Proc::PID::File::Fcntl (0)
---> Complete! 1 cpanfile dependencies. 1 modules installed.

The (0) sounds a little alarming.

miyagawa commented 2 years ago

OK, your report might have been inaccurate and I can reproduce this with a minimum version:

➜  cat cpanfile
requires 'Proc::PID::File::Fcntl', '1.01';

➜  carmel install
---> Installing new dependencies: Proc::PID::File::Fcntl
Successfully installed Proc-PID-File-Fcntl-1.01
1 distribution installed
Can't find an artifact for Proc::PID::File::Fcntl => 1.01
You need to run `carmel install` first to get the modules installed and artifacts built.

And this must be why:

builds/Proc-PID-File-Fcntl-1.01 
➜  jq . blib/meta/install.json
{
  "provides": {
    "Proc::PID::File::Fcntl": {
      "file": "Fcntl.pm.PL"
    }
  },
  "target": "Proc::PID::File::Fcntl",
  "name": "Proc::PID::File::Fcntl",
  "version": "1.01",
  "pathname": "J/JG/JGMYERS/Proc-PID-File-Fcntl-1.01.tar.gz",
  "dist": "Proc-PID-File-Fcntl-1.01"
}

This is one of those weird modules which generates the .pm files out of .pm.PL thus Module::Metadata is failing to extract the version from.

It's a little hard to fix without fixing it upstream (or fixing it in Menlo with Module::Metadata and Parse::PMFile if I recall correctly).

For now the workaround should be:

Grinnz commented 2 years ago

I am not using a version requirement in cpanfile.

Carmel version v0.1.56 This is perl 5, version 34, subversion 1 (v5.34.1) built for x86_64-linux

miyagawa commented 2 years ago

I am not using a version requirement in cpanfile.

Ah, I guess I get it now. I didn't get this because I had an existing cpanfile.snapshot file so adding a new module to cpanfile wouldn't cause the "make sure it gets the version I just got from CPAN" path (which, by itself can be considered a bug due to inconsistency, see below).

Here's a terrible workaround to avoid that path, for now:

  1. create an empty cpanfile, run carmel install
  2. Add that dependency to cpanfile without a version requirement, run carmel install

I confirmed that this wouldn't complain. This might not work if you have a transient dependency that depends on this module with a non-zero version.

The reason that this works (at least for now) is not very important for this issue, but in case you're curious -- it is that there's this logic implemented in #53 where you run carmel install for the first time without snapshot, it makes sure it gets the latest version (1.01) from CPAN, and then validates that the versions are up-to-date, which fails due to that bad install.json metadata from Menlo. This validation process is skipped if you add a new dependency to existing cpanfile + snapshot, which is a bug on its own (#65)

Either way the fundamental issue is that it creates an invalid install metadata that doesn't have the correct version metadata in provides so Carmel can't verify it.

miyagawa commented 2 years ago

Seems to be a classic problem: https://github.com/perl-carton/carton/issues/210

I haven't read all the discussion, but it seems it was dealt with in the upstream rather than fixing it in cpanm/Menlo.

https://metacpan.org/dist/Proc-PID-File-Fcntl/source This module doesn't even have any META files and no .pm file, so it makes me wonder how PAUSE managed to index it anyway.

Grinnz commented 2 years ago

It was uploaded probably before META existed - META is technically still not required, and provides certainly isn't, PAUSE is capable of analyzing the .PL file if needed.

miyagawa commented 2 years ago

Yeah, it's possible for Carton to workaround this by "upgrading" the install.json after scanning the blib directory, or also checking .pm.PL files, but it's best to fix this in upstream. Also I'll look into providing a way to locally override the lookup so you can specify the patched distribution in cpanfile (#72)