Sanitizer of record fields?

Sanitizer of record fields?

[zimoun]

Hi,

The website is currently failing [1] to build because a typo in some
package declaration.  The error message is not very helpful,

        srfi/srfi-1.scm:241:2: In procedure map:
        In procedure map: Wrong type argument: "https://www.qt.io/"
        building pages in '/tmp/gnu.org/software/guix'...

and it was not straightforward to find the issue.  Using some ’pk’ in
the website builder restricted the origin of the failure; but still.
Thanks to Florian, they found this commit [2] introducing the package
qtshadertools where a field is unexpected,

        +    (license (package-home-page qtbase))))

and boum!

It seems impossible to detect that typo at compile-time because fields
do not have a specific type (except by convention).  Therefore, how can
we detect such typo?

We could add a lint checker.  Is it a “good” idea?

Because lint is not always applied, a check should be done when running
’make’ or a special target.  Is it a “good” idea?


1: <http://issues.guix.gnu.org/issue/57581>
2: <https://git.savannah.gnu.org/cgit/guix.git/diff/?id=1d65ff8fdeb20cc2db956093f0ecb1f3f72afc0e>


Cheers,
simon

Re: Sanitizer of record fields?

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1294 bytes --]


On 08-09-2022 09:59, zimoun wrote:
> We could add a lint checker.  Is it a “good” idea?
We already have one, 'check-license'.
> Because lint is not always applied, a check should be done when running
> ’make’ or a special target.  Is it a “good” idea?

I suppose it is a possibility, but it adds a few seconds to every 'make':

time ./pre-inst-env guix lint --checkers=license
make  all-recursive  [...]
gnu/packages/qt.scm:1373:13: qtshadertools@6.3.1: invalid license field
gnu/packages/tex.scm:11816:2: texlive-setspace@59745: invalid

real    0m1,492s
user    0m3,331s
sys    0m0,214s

As such, here's an alternative proposal: instead of checking it at 
compile-time
(which is currently impossible), let's check them at runtime, with a 
field sanitizer.

Given that, unless I'm mistaken, build-aux/compile-all.scm loads every Guix
module anyway, and given that 'license' isn't thunked or delayed, 'the 
runtime
check' would also a compile-time check.

(Performance impact on "guix ..." commands would need to be checked.)

Alternatively, some error checking could be added to the website code, 
to indicate
which package and which is wrong.  Or maybe the website code can run the 
'license'
linter first.

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Sanitizer of record fields?

[zimoun]

Hi,

On Thu, 08 Sep 2022 at 11:32, Maxime Devos <maximedevos@telenet.be> wrote:
> On 08-09-2022 09:59, zimoun wrote:

>> We could add a lint checker.  Is it a “good” idea?
>
> We already have one, 'check-license'.

Yeah, but I was talking about check if the field return the expected
record type.  Not only for the license one.

>> Because lint is not always applied, a check should be done when running
>> ’make’ or a special target.  Is it a “good” idea?
>
> I suppose it is a possibility, but it adds a few seconds to every 'make':
>
> time ./pre-inst-env guix lint --checkers=license
> make  all-recursive  [...]
> gnu/packages/qt.scm:1373:13: qtshadertools@6.3.1: invalid license field
> gnu/packages/tex.scm:11816:2: texlive-setspace@59745: invalid

It means that “guix lint” is not systematically run. :-)

I agree that the overhead on ’make’ is probably not worth.  Maybe, more
checks could be run by the pre push hook.  WDYT?


Cheers,
simon

Re: Sanitizer of record fields?

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1276 bytes --]


On 08-09-2022 13:16, zimoun wrote:
> Hi,
>
> On Thu, 08 Sep 2022 at 11:32, Maxime Devos <maximedevos@telenet.be> wrote:
>> On 08-09-2022 09:59, zimoun wrote:
>>> We could add a lint checker.  Is it a “good” idea?
>> We already have one, 'check-license'.
> Yeah, but I was talking about check if the field return the expected
> record type.  Not only for the license one.
Then maybe add more checkers, for the other fields as well? I'm not seeing
any downsides.
>>> Because lint is not always applied, a check should be done when running
>>> ’make’ or a special target.  Is it a “good” idea?
>> I suppose it is a possibility, but it adds a few seconds to every 'make':
>>
>> time ./pre-inst-env guix lint --checkers=license
>> make  all-recursive  [...]
>> gnu/packages/qt.scm:1373:13: qtshadertools@6.3.1: invalid license field
>> gnu/packages/tex.scm:11816:2: texlive-setspace@59745: invalid
> It means that “guix lint” is not systematically run. :-)
>
> I agree that the overhead on ’make’ is probably not worth.  Maybe, more
> checks could be run by the pre push hook.  WDYT?
I suppose?  Not seeing any immediate problems, but I haven't ever pushed
to the Guix repo or ever used pre-push hooks ...

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Sanitizer of record fields?

[bokr]

Hi Simon, et al

On +2022-09-08 09:59:15 +0200, zimoun wrote:
> Hi,
> 
> The website is currently failing [1] to build because a typo in some
> package declaration.  The error message is not very helpful,
> 
>         srfi/srfi-1.scm:241:2: In procedure map:
>         In procedure map: Wrong type argument: "https://www.qt.io/"
>         building pages in '/tmp/gnu.org/software/guix'...
>

ISTM this "wrong type argument" is an infuriatingly common
and typically useless error message.

Would it be possible to have a debugging hook where the message is output
which if activated would show the call stack, so one could see where in
the user code it happened?

Activation could I imagine be by guile checking e.g.
"GUILE_THROW_DEBUG_WRONG_ARGUMENT" on invocation, to turn on the hook
in a way that would have zero performance effect if not activated.

Alternatively, could a wrapper start one's problem code using GDB
to put a break point at the hook place, pre-setting up the call
args etc and starting GDB so you could type run or step etc.?

Can geiser trace stuff? IWBN to have something analogous to bash's
shopt for printing expression sources as they are read and/or executed.
Does something like that exist?

Thoughts? :)

> and it was not straightforward to find the issue.  Using some ’pk’ in
> the website builder restricted the origin of the failure; but still.
> Thanks to Florian, they found this commit [2] introducing the package
> qtshadertools where a field is unexpected,
> 
>         +    (license (package-home-page qtbase))))
> 
> and boum!
> 
> It seems impossible to detect that typo at compile-time because fields
> do not have a specific type (except by convention).  Therefore, how can
> we detect such typo?
> 
> We could add a lint checker.  Is it a “good” idea?
> 
> Because lint is not always applied, a check should be done when running
> ’make’ or a special target.  Is it a “good” idea?
> 
> 
> 1: <http://issues.guix.gnu.org/issue/57581>
> 2: <https://git.savannah.gnu.org/cgit/guix.git/diff/?id=1d65ff8fdeb20cc2db956093f0ecb1f3f72afc0e>
> 
> 
> Cheers,
> simon
>
--
Regards,
Bengt Richter

Re: Sanitizer of record fields?

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 2198 bytes --]


On 08-09-2022 13:35, bokr@bokr.com wrote:
> Hi Simon, et al
>
> On +2022-09-08 09:59:15 +0200, zimoun wrote:
>> Hi,
>>
>> The website is currently failing [1] to build because a typo in some
>> package declaration.  The error message is not very helpful,
>>
>>          srfi/srfi-1.scm:241:2: In procedure map:
>>          In procedure map: Wrong type argument: "https://www.qt.io/"
>>          building pages in '/tmp/gnu.org/software/guix'...
>>
> ISTM this "wrong type argument" is an infuriatingly common
> and typically useless error message.
>
> Would it be possible to have a debugging hook where the message is output
> which if activated would show the call stack, so one could see where in
> the user code it happened?
>
> Activation could I imagine be by guile checking e.g.
> "GUILE_THROW_DEBUG_WRONG_ARGUMENT" on invocation, to turn on the hook
> in a way that would have zero performance effect if not activated.

Why a hook / environment variable in Guile? Backtraces are printed
by default, I do not see the benefit of disabling them by default and
adding an environment variable to enable them:

> antipode@antipode ~$ guile -c '(map 0 1)'
> Backtrace:
> In ice-9/boot-9.scm:
>   1752:10  6 (with-exception-handler _ _ #:unwind? _ # _)
> In unknown file:
>            5 (apply-smob/0 #<thunk 7fa30cc882e0>)
> In ice-9/boot-9.scm:
>     724:2  4 (call-with-prompt ("prompt") #<procedure 7fa30cc9acc0 …> …)
> In ice-9/eval.scm:
>     619:8  3 (_ #(#(#<directory (guile-user) 7fa30cc8dc80>)))
> In ice-9/command-line.scm:
>    185:19  2 (_ #<input: string 7fa30cc87850>)
> In unknown file:
>            1 (eval (map 0 1) #<directory (guile-user) 7fa30cc8dc80>)
> In ice-9/boot-9.scm:
>     218:9  0 (map 0 1)
>
> ice-9/boot-9.scm:218:9: In procedure map:
> In procedure map: Not a list: 1
Problem is already solved.

It appears that the website code catches exceptions and prints the error 
message,
but forgets to print the backtrace, but that seems to be a choice that 
the website
code made, not Guile. (Likely, an accidental choice, that can be improved.)

Greetings,
Maxime.



[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Sanitizer of record fields?

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 340 bytes --]


On 08-09-2022 13:35, bokr@bokr.com wrote:
> Can geiser trace stuff? IWBN to have something analogous to bash's
> shopt for printing expression sources as they are read and/or executed.
> Does something like that exist?

See: 'trace' in the Guile manual.  (This is a Guile feature, not a 
geiser feature.)

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: Sanitizer of record fields?

[zimoun]

Hi,

On Thu, 8 Sept 2022 at 13:36, <bokr@bokr.com> wrote:

> >         srfi/srfi-1.scm:241:2: In procedure map:
> >         In procedure map: Wrong type argument: "https://www.qt.io/"
> >         building pages in '/tmp/gnu.org/software/guix'...
> >
>
> ISTM this "wrong type argument" is an infuriatingly common
> and typically useless error message.

The complete backtrace is showed in the bug report; see:

    <https://issues.guix.gnu.org/issue/57581#0>

I just pasted what I consider as the relevant snippet.

> Would it be possible to have a debugging hook where the message is output
> which if activated would show the call stack, so one could see where in
> the user code it happened?

To my knowledge, it is not possible.  Note that here Guix is used as a
library called by 'haunt'.


Cheers,
simon

Re: Sanitizer of record fields?

[Ludovic Courtès]

Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

> The website is currently failing [1] to build because a typo in some
> package declaration.  The error message is not very helpful,
>
>         srfi/srfi-1.scm:241:2: In procedure map:
>         In procedure map: Wrong type argument: "https://www.qt.io/"
>         building pages in '/tmp/gnu.org/software/guix'...
>
> and it was not straightforward to find the issue.  Using some ’pk’ in
> the website builder restricted the origin of the failure; but still.
> Thanks to Florian, they found this commit [2] introducing the package
> qtshadertools where a field is unexpected,
>
>         +    (license (package-home-page qtbase))))
>
> and boum!
>
> It seems impossible to detect that typo at compile-time because fields
> do not have a specific type (except by convention).  Therefore, how can
> we detect such typo?

I’m late to the party, but I have a proposal:

  https://issues.guix.gnu.org/58231

Hope you like it!

Ludo’.