bug#49827: Error message for missing synopsis in opam importer

bug#49827: Error message for missing synopsis in opam importer

[Alice BRENON]

Hello,

I triggered a confusing behaviour from the opam importer trying to
import package iter 1.2.1 today on a Guix System install.

The package iter is missing a "synopsis" field as can be seen on
https://opam.ocaml.org/packages/iter/ , which when I tried

guix import opam iter

yielded the following backtrace:

Backtrace:
           8 (primitive-load "/home/alice/.config/guix/current/bin/g…")
In guix/ui.scm:
   2185:7  7 (run-guix . _)
  2148:10  6 (run-guix-command _ . _)
In guix/scripts/import.scm:
   120:11  5 (guix-import . _)
In guix/scripts/import/opam.scm:
   104:23  4 (guix-import-opam . _)
In guix/utils.scm:
    752:8  3 (call-with-temporary-output-file _)
In guix/import/opam.scm:
   337:34  2 (_ _ _)
In srfi/srfi-1.scm:
   460:18  1 (fold #<procedure 7f3baca56fe0 at guix/import/opam.scm…> …)
In guix/import/opam.scm:
   193:15  0 (_ _ _)

guix/import/opam.scm:193:15: Throw to key `match-error' with args
`("match" "no matching pattern" string-pat)'.


the final error is raised l.193 of guix/import/opam.scm because
metadata-ref supports various types for a metadata field, but not the
lack of it. As discussed with Maxime Devos on the IRC channel, it would
be helpful to either allow the import of a package with a missing field
(possibly filling it in the output scheme code for the imported package
with some bad value requiring the user to fill it and causing any build
to crash until replaced properly) or at least to handle that missing
field with a more explicit error message than the above backtrace
(something like "Can't import that package because it's missing such or
such field").

Thanks,

Alice BRENON

bug#49827: Error message for missing synopsis in opam importer

[Sarah Morgensen]

Hi,

Thanks for the report. I'm CC'ing Simon since they have been working on
improved error handling/reporting for the importers.

Alice BRENON <alice.brenon@ens-lyon.fr> writes:

> Hello,
>
> I triggered a confusing behaviour from the opam importer trying to
> import package iter 1.2.1 today on a Guix System install.
>
> The package iter is missing a "synopsis" field as can be seen on
> https://opam.ocaml.org/packages/iter/ , which when I tried
>
> guix import opam iter
>
> yielded the following backtrace:
>
> Backtrace:
>            8 (primitive-load "/home/alice/.config/guix/current/bin/g…")
> In guix/ui.scm:
>    2185:7  7 (run-guix . _)
>   2148:10  6 (run-guix-command _ . _)
> In guix/scripts/import.scm:
>    120:11  5 (guix-import . _)
> In guix/scripts/import/opam.scm:
>    104:23  4 (guix-import-opam . _)
> In guix/utils.scm:
>     752:8  3 (call-with-temporary-output-file _)
> In guix/import/opam.scm:
>    337:34  2 (_ _ _)
> In srfi/srfi-1.scm:
>    460:18  1 (fold #<procedure 7f3baca56fe0 at guix/import/opam.scm…> …)
> In guix/import/opam.scm:
>    193:15  0 (_ _ _)
>
> guix/import/opam.scm:193:15: Throw to key `match-error' with args
> `("match" "no matching pattern" string-pat)'.
>
>
> the final error is raised l.193 of guix/import/opam.scm because
> metadata-ref supports various types for a metadata field, but not the
> lack of it. As discussed with Maxime Devos on the IRC channel, it would
> be helpful to either allow the import of a package with a missing field
> (possibly filling it in the output scheme code for the imported package
> with some bad value requiring the user to fill it and causing any build
> to crash until replaced properly) or at least to handle that missing
> field with a more explicit error message than the above backtrace
> (something like "Can't import that package because it's missing such or
> such field").

IMO, a warning should be emitted, but the package should be buildable if
at all possible; it's the submitter's responsibility to vet imported
packages.

Simon, how's that error handling rework coming? ;)

>
> Thanks,
>
> Alice BRENON

--
Sarah

bug#49827: Error message for missing synopsis in opam importer

[Alice BRENON]

Hello,

Thanks for your answer Sarah. Simon, I don't know if you have been able
to make any progress but I wanted to make sure you had seen the patch
proposal I sent to let the opam importer work from more repositories
than the few initially defined (opam's official and three for coq):

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=49958

Though I had a local "bypass" on the metadata reader to be able to
perform the import I wanted and orginally designed my patch for, I paid
attention not to commit it to keep matters separated. Any insight on the
general form the improved error handling will take ? Please let me
know if I can update my #49958 patch to play along more nicely with
your rework.


Alice

Le Mon, 02 Aug 2021 12:28:20 -0700,
Sarah Morgensen <iskarian@mgsn.dev> a écrit :

> Hi,
> 
> Thanks for the report. I'm CC'ing Simon since they have been working
> on improved error handling/reporting for the importers.
> 
> Alice BRENON <alice.brenon@ens-lyon.fr> writes:
> 
> > Hello,
> >
> > I triggered a confusing behaviour from the opam importer trying to
> > import package iter 1.2.1 today on a Guix System install.
> >
> > The package iter is missing a "synopsis" field as can be seen on
> > https://opam.ocaml.org/packages/iter/ , which when I tried
> >
> > guix import opam iter
> >
> > yielded the following backtrace:
> >
> > Backtrace:
> >            8 (primitive-load
> > "/home/alice/.config/guix/current/bin/g…") In guix/ui.scm:
> >    2185:7  7 (run-guix . _)
> >   2148:10  6 (run-guix-command _ . _)
> > In guix/scripts/import.scm:
> >    120:11  5 (guix-import . _)
> > In guix/scripts/import/opam.scm:
> >    104:23  4 (guix-import-opam . _)
> > In guix/utils.scm:
> >     752:8  3 (call-with-temporary-output-file _)
> > In guix/import/opam.scm:
> >    337:34  2 (_ _ _)
> > In srfi/srfi-1.scm:
> >    460:18  1 (fold #<procedure 7f3baca56fe0 at
> > guix/import/opam.scm…> …) In guix/import/opam.scm:
> >    193:15  0 (_ _ _)
> >
> > guix/import/opam.scm:193:15: Throw to key `match-error' with args
> > `("match" "no matching pattern" string-pat)'.
> >
> >
> > the final error is raised l.193 of guix/import/opam.scm because
> > metadata-ref supports various types for a metadata field, but not
> > the lack of it. As discussed with Maxime Devos on the IRC channel,
> > it would be helpful to either allow the import of a package with a
> > missing field (possibly filling it in the output scheme code for
> > the imported package with some bad value requiring the user to fill
> > it and causing any build to crash until replaced properly) or at
> > least to handle that missing field with a more explicit error
> > message than the above backtrace (something like "Can't import that
> > package because it's missing such or such field").  
> 
> IMO, a warning should be emitted, but the package should be buildable
> if at all possible; it's the submitter's responsibility to vet
> imported packages.
> 
> Simon, how's that error handling rework coming? ;)
> 
> >
> > Thanks,
> >
> > Alice BRENON  
> 
> --
> Sarah

bug#49827: Error message for missing synopsis in opam importer

[zimoun]

Hi,

I am back from holidays. :-)

On Wed, 11 Aug 2021 at 15:15, Alice BRENON <alice.brenon@ens-lyon.fr> wrote:

> Thanks for your answer Sarah. Simon, I don't know if you have been able
> to make any progress but I wanted to make sure you had seen the patch
> proposal I sent to let the opam importer work from more repositories
> than the few initially defined (opam's official and three for coq):
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=49958

I have seen but not read in details.  I will do.

>> > The package iter is missing a "synopsis" field as can be seen on
>> > https://opam.ocaml.org/packages/iter/ , which when I tried
>> >
>> > guix import opam iter
>> >
>> > yielded the following backtrace:
>> >
>> > Backtrace:
>> >            8 (primitive-load
>> > "/home/alice/.config/guix/current/bin/g…") In guix/ui.scm:
>> >    2185:7  7 (run-guix . _)
>> >   2148:10  6 (run-guix-command _ . _)
>> > In guix/scripts/import.scm:
>> >    120:11  5 (guix-import . _)
>> > In guix/scripts/import/opam.scm:
>> >    104:23  4 (guix-import-opam . _)
>> > In guix/utils.scm:
>> >     752:8  3 (call-with-temporary-output-file _)
>> > In guix/import/opam.scm:
>> >    337:34  2 (_ _ _)
>> > In srfi/srfi-1.scm:
>> >    460:18  1 (fold #<procedure 7f3baca56fe0 at
>> > guix/import/opam.scm…> …) In guix/import/opam.scm:
>> >    193:15  0 (_ _ _)
>> >
>> > guix/import/opam.scm:193:15: Throw to key `match-error' with args
>> > `("match" "no matching pattern" string-pat)'.
>> >
>> >
>> > the final error is raised l.193 of guix/import/opam.scm because
>> > metadata-ref supports various types for a metadata field, but not
>> > the lack of it. As discussed with Maxime Devos on the IRC channel,
>> > it would be helpful to either allow the import of a package with a
>> > missing field (possibly filling it in the output scheme code for
>> > the imported package with some bad value requiring the user to fill
>> > it and causing any build to crash until replaced properly) or at
>> > least to handle that missing field with a more explicit error
>> > message than the above backtrace (something like "Can't import that
>> > package because it's missing such or such field").  

From my understanding, there is 2 issues:

 - gentle handler for error
 - warn for incomplete metadata

With Jérémy (jeko), we have started to work time to time using
experimental pair-programming to fix the former.  Currently, each
importer uses its own error mechanism and obviously incoherence between
them happens; especially when ’--recursive’.  We are trying to unify
that.

Thanks for the report of this use case. :-)


Cheers,
simon

bug#49827: Error message for missing synopsis in opam importer

[Alice BRENON]

Hello,

Thanks for your answer !

Le Tue, 17 Aug 2021 09:43:10 +0200,
zimoun <zimon.toutoune@gmail.com> a écrit :

> Hi,
> 
> I am back from holidays. :-)
> 
> …   
> 
> From my understanding, there is 2 issues:  
> 
>  - gentle handler for error
>  - warn for incomplete metadata
> 

Yes, absolutely, because currently understanding the cause of the error
requires to delve into the source to understand what is going on. The
warning part is more optional, but if this pattern matching is modified
to handle that special case of a missing metadata instead of entirely
crashing, I thought it could be useful not to be too permissive either,
and to at least mention that a missing metadata was caught and should
be filled by hand.

This could take the form of a message above the output of the actual
scheme code for the package declaration while the importer is running,
or of an invalid value for that missing field in the generated scheme
output, something like "<FILL-ME>" or such that would be invalid in
scheme and would make guix build fail when trying to use the output
directly without manually editing it to fill the missing metadata.

> With Jérémy (jeko), we have started to work time to time using
> experimental pair-programming to fix the former.  Currently, each
> importer uses its own error mechanism and obviously incoherence
> between them happens; especially when ’--recursive’.  We are trying
> to unify that.
> 
> Thanks for the report of this use case. :-)

Glad to learn my report could help : )

> 
> 
> Cheers,
> simon

bug#49827: Error message for missing synopsis in opam importer

[Julien Lepiller]

Hi,

First, today when running guix import opam iter, I get a synopsis, and
#f as the description because the field is missing.

I also pushed a small patch to master, as
24aa7b3c21309b63cc6e8e18d6417d2cddccf6c6, that ensures that, when the
field exists but contains unknown data, we also return #f instead of a
match error that produces the ugly backtrace you saw.

Thanks for the report, closing :)