GNU bug report logs - #49315
[PATCH]: Lint usages of 'wrap-program' without a "bash" input.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49315 in the body.
You can then email your comments to 49315 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: guix-patches <at> gnu.org
Subject: [PATCH]: Lint usages of 'wrap-program' without a "bash" input.
Date: Thu, 01 Jul 2021 13:39:53 +0200

[Message part 1 (text/plain, inline)]

Hi Guix,

These two patches add a 'wrapper-inputs' linter.
It detects if "wrap-program" is used without adding
"bash" or "bash-minimal" to 'inputs'. Adding "bash"
or "bash-minimal" is necessary when cross-compiling,
otherwise the resulting wrapper will use an interpreter
for the wrong architecture.

This linter detects 365 problematic packages.

Greetings,
Maxime.

[0001-lint-Define-some-procedures-for-analysing-code-in-ph.patch (text/x-patch, attachment)]

[0002-lint-Lint-usages-of-wrap-program-without-a-bash-inpu.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 49315 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 49315 <at> debbugs.gnu.org
Subject: Re: [bug#49315] [PATCH]: Lint usages of 'wrap-program' without a
 "bash" input.
Date: Tue, 06 Jul 2021 19:29:07 +0200

Hey Maxim,

> +  "Try to find the body of the procedure defined inline by EXPRESSION.
> +If it was found, call EXPRESSION with its body. If it wasn't, call
                               ^
                               FOUND

> +    (`(,(or 'let 'let*) . ,_)
> +     (find-procedure-body (car (last-pair expression)) found
> +                          #:not-found not-found))

You can use "last" from (srfi srfi-1) here. What's the point of
stripping the let clause by the way?

> +  (list (make-warning package
> +                      ;; TRANSLATORS: 'modify-phases' is a Scheme syntax
> +                      ;; and should not be translated.
> +                      (G_ "incorrect call to ‘modify-phases’")
> +                      #:field 'arguments)))

Maybe you could return a plain object here.

> +  (list (make-warning package
> +                      ;; TRANSLATORS: See ‘modify-phases’ in the manual.
> +                      (G_ "invalid phase clause")
> +                      #:field 'arguments)))

and here.

Thanks,

Mathieu

Message #11 received at 49315 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 49315 <at> debbugs.gnu.org
Subject: Re: [bug#49315] [PATCH]: Lint usages of 'wrap-program' without a
 "bash" input.
Date: Tue, 06 Jul 2021 19:43:19 +0200

> When using 'wrap-program', "bash" (or "bash-minimal") should be
> in inputs.  Otherwise, when cross-compiling, 'wrap-program' will use
> a native bash instead of the cross bash and the 'patch-shebangs' won't
> be able to correct this.

Nice one! Seems to work fine, and as most packages that use wrap-program
are broken in that aspect, that's a welcomed addition.

Sorry for misspelling your first name in my last email.

Thanks,

Mathieu

Message #14 received at 49315 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Mathieu Othacehe <othacehe <at> gnu.org>
Cc: 49315 <at> debbugs.gnu.org
Subject: Re: [bug#49315] [PATCH]: Lint usages of 'wrap-program' without a
 "bash" input.
Date: Tue, 06 Jul 2021 22:38:03 +0200

[Message part 1 (text/plain, inline)]

Mathieu Othacehe schreef op di 06-07-2021 om 19:29 [+0200]:
> Hey Maxime,
> 
> > +  "Try to find the body of the procedure defined inline by EXPRESSION.
> > +If it was found, call EXPRESSION with its body. If it wasn't, call
>                                ^
>                                FOUND
> 
> > +    (`(,(or 'let 'let*) . ,_)
> > +     (find-procedure-body (car (last-pair expression)) found
> > +                          #:not-found not-found))
> 
> You can use "last" from (srfi srfi-1) here.

No, I can't -- unless a backtrace in case of invalid code is acceptable.
The problem is that the procedure 'last' expects its argument to be a list.
E.g., try from a REPL:

scheme@(guile-user)> ((@ (srfi srfi-1) last) '("a" . 0))
$1 = 0

... wait, why didn't this raise an exception?
Looking at the definition of 'last' in (srfi srfi-1) in guile, I see

(define (last pair)
  "Return the last element of the non-empty, finite list PAIR."
  (car (last-pair pair)))

So, you're correct that "last" from (srfi srfi-1) can be used here,
but this still seems rather fragile to me and an implementation detail
of (srfi srfi-1).

> What's the point of stripping the let clause by the way?

Because 'find-procedure-body' is supposed to find the body of the procedure,
but sometimes the lambda is wrapped in a 'let'. (I don't have an example
currently in mind, but I've definitely seen things like

  (modify-phases %standard-phases
    (replace 'something
      (let ((three (+ 1 2))
        (lambda _
          (format #t "~a~%" tree)))))


in package definitions).  You could ask, why do we need to extract the
procedure body, can't we just pass the whole expression as-is to
'check-procedure-body'?

That's a possiblity, but would lead to some false negatives: consider the
following phases definition:

  (modify-phases %standard-phases
    (replace 'check
      (let ((three (+ 1 2)))
        (lambda* (#:key tests? #:allow-other-keys)
          (invoke "stuff" (number->string bla-bla))))))

In this case, the parameter tests? is ignored, even though the symbol 'tests?'
appears in the expression.  So we shouldn't look at the parameter.

--- wait, this is a patch review for the 'wrapper-inputs' linter, not the
'optional-tests' linter!  Indeed, for the 'wrapper-inputs' linter, stripping
the 'let' (or 'let*') is pointless.  It might even lead to false negatives!

Consider the case

    (add-after 'install 'wrap
      (let ((wrap (lambda (x) (wrap-program x [...]))))
        (lambda _
          (wrap "stuff")
          (wrap "other-stuff"))))

That usage should ideally be detected by 'wrap-program'.  But as no
current package definition seems to do such a thing, and using
'find-procedure-body' seems marginally ‘cleaner’ to me (YMMV?),
I would use 'find-procedure-body' anyways.

> > +  (list (make-warning package
> > +                      ;; TRANSLATORS: 'modify-phases' is a Scheme syntax
> > +                      ;; and should not be translated.
> > +                      (G_ "incorrect call to ‘modify-phases’")
> > +                      #:field 'arguments)))
> 
> Maybe you could return a plain object here.

Yes, but then

(define* (find-phase-deltas package found
                            #:key (not-found (const '()))
                            (bogus (cut report-bogus-phase-deltas package <>)))

would need to become

(define* (find-phase-deltas package found
                            #:key (not-found (const '()))
                            (bogus (lambda (bogus-deltas)
                                     (list (report-bogus-deltas package bogus-deltas)))))

which is rather verbose. (The 'bogus-deltas' argument could be dropped I suppose?
But 'find-phase-deltas' is supposed to be generic, such that its callers can have
easy access to the bogus (dotted) list of phases ...)


> > +  (list (make-warning package
> > +                      ;; TRANSLATORS: See ‘modify-phases’ in the manual.
> > +                      (G_ "invalid phase clause")
> > +                      #:field 'arguments)))
> 
> and here.

Likewise.

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 49315 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Mathieu Othacehe <othacehe <at> gnu.org>
Cc: 49315 <at> debbugs.gnu.org
Subject: Re: [bug#49315] [PATCH]: Lint usages of 'wrap-program' without a
 "bash" input.
Date: Tue, 06 Jul 2021 22:51:29 +0200

[Message part 1 (text/plain, inline)]

Mathieu Othacehe schreef op di 06-07-2021 om 19:43 [+0200]:
> > When using 'wrap-program', "bash" (or "bash-minimal") should be
> > in inputs.  Otherwise, when cross-compiling, 'wrap-program' will use
> > a native bash instead of the cross bash and the 'patch-shebangs' won't
> > be able to correct this.
> 
> Nice one! Seems to work fine, and as most packages that use wrap-program
> are broken in that aspect, that's a welcomed addition.

Thanks!  I sent a patch series lately adding "bash-minimal" to 'inputs'
when 'wrap-program' is used, using this linter to identify relevant cases.
Ludo would merge it (on core-updates, after some other things have been fixed
there).

I noticed 'qt-build-system' uses 'wrap-program' in its 'qt-wrap'
phase, so I've sent a patch to add 'bash-minimal' to the bag of 'qt-build-system'
when cross-compiling.  That should largely take care of the 'wrap-program'
cross-compilation issues, I hope!

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #22 received at 49315-done <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 49315-done <at> debbugs.gnu.org
Subject: Re: [bug#49315] [PATCH]: Lint usages of 'wrap-program' without a
 "bash" input.
Date: Wed, 07 Jul 2021 11:12:33 +0200

Hey,

> That usage should ideally be detected by 'wrap-program'.  But as no
> current package definition seems to do such a thing, and using
> 'find-procedure-body' seems marginally ‘cleaner’ to me (YMMV?),
> I would use 'find-procedure-body' anyways.

OK, seems fair. I just edited the first patch to use "last" that has the
benefit of hiding a "car" call and wrapped a long line.

Pushed on master.

Thanks,

Mathieu

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.