unofficial mirror of bug-guix@gnu.org 
 help / color / mirror / Atom feed
* bug#49827: Error message for missing synopsis in opam importer
@ 2021-08-02 15:01 Alice BRENON
  2021-08-02 19:28 ` Sarah Morgensen
  0 siblings, 1 reply; 6+ messages in thread
From: Alice BRENON @ 2021-08-02 15:01 UTC (permalink / raw)
  To: 49827

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#49827: Error message for missing synopsis in opam importer
  2021-08-02 15:01 bug#49827: Error message for missing synopsis in opam importer Alice BRENON
@ 2021-08-02 19:28 ` Sarah Morgensen
  2021-08-11 13:15   ` Alice BRENON
  0 siblings, 1 reply; 6+ messages in thread
From: Sarah Morgensen @ 2021-08-02 19:28 UTC (permalink / raw)
  To: Alice BRENON; +Cc: 49827

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#49827: Error message for missing synopsis in opam importer
  2021-08-02 19:28 ` Sarah Morgensen
@ 2021-08-11 13:15   ` Alice BRENON
  2021-08-17  7:43     ` zimoun
  0 siblings, 1 reply; 6+ messages in thread
From: Alice BRENON @ 2021-08-11 13:15 UTC (permalink / raw)
  To: Sarah Morgensen; +Cc: 49827

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





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#49827: Error message for missing synopsis in opam importer
  2021-08-11 13:15   ` Alice BRENON
@ 2021-08-17  7:43     ` zimoun
  2021-08-19 14:58       ` Alice BRENON
  0 siblings, 1 reply; 6+ messages in thread
From: zimoun @ 2021-08-17  7:43 UTC (permalink / raw)
  To: Alice BRENON, Sarah Morgensen; +Cc: 49827

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#49827: Error message for missing synopsis in opam importer
  2021-08-17  7:43     ` zimoun
@ 2021-08-19 14:58       ` Alice BRENON
  2021-11-21 23:22         ` Julien Lepiller
  0 siblings, 1 reply; 6+ messages in thread
From: Alice BRENON @ 2021-08-19 14:58 UTC (permalink / raw)
  To: zimoun; +Cc: Sarah Morgensen, 49827

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





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#49827: Error message for missing synopsis in opam importer
  2021-08-19 14:58       ` Alice BRENON
@ 2021-11-21 23:22         ` Julien Lepiller
  0 siblings, 0 replies; 6+ messages in thread
From: Julien Lepiller @ 2021-11-21 23:22 UTC (permalink / raw)
  To: Alice BRENON; +Cc: Sarah Morgensen, 49827-done

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 :)




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-21 23:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-02 15:01 bug#49827: Error message for missing synopsis in opam importer Alice BRENON
2021-08-02 19:28 ` Sarah Morgensen
2021-08-11 13:15   ` Alice BRENON
2021-08-17  7:43     ` zimoun
2021-08-19 14:58       ` Alice BRENON
2021-11-21 23:22         ` Julien Lepiller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).