Re: [PATCH] Add tcsh.

[Cyril Roelandt]

On 02/04/2013 11:32 PM, Ludovic Courtès wrote:
> Cyril Roelandt<address@hidden>  skribis:
>
> > This patch adds tcsh. It was a bit hard to make the testsuite work: I disabled a
> > few tests that I could not get working during the "check" phase, but it should
> > not be a problem, since they work fine with the installed binary.
>
> Good!

Not so good: it's way too weird that some tests fail when running "make 
check", it'd be great to understand why.

> > +    (inputs
> > +     `(("autoconf" ,autoconf)
> > +       ("coreutils" ,coreutils)
> > +       ("ncurses" ,ncurses)
> > +       ("patch/skip-tests"
> > +        ,(search-patch "tcsh-fix-autotest.patch"))))
>
> In general, rebuilding the build infrastructure with Autoconf&
> co. should be avoided for several reasons: we may get it wrong, and it
> will yield a rebuild on every Autoconf update.
>
> Could this be easily avoided here?  (I suspect you already tried...)
> One option would be to make the patch against ‘testsuite’ instead of
> against the .at files, with the risk of it no longer being applicable on
> the next release.
>
> WDYT?

It would make sense patch the .at files, generate a new "testsuite" 
file, and patch "testsuite" when running make check. But it would be 
nice to keep the *.at files somewhere to quickly regenerate the patch 
against "testsuite" when a new version of tcsh comes out. Can we do that ?

> > +        (lambda* (#:key inputs #:allow-other-keys #:rest args)
> > +          (let ((check (assoc-ref %standard-phases 'check)))
> > +            ;; Take care of pwd
> > +            (substitute* "tests/commands.at" (("/bin/pwd") (which "pwd")))
> > +            (substitute* "tests/variables.at" (("/bin/pwd") (which "pwd")))
>
> ‘substitute*’ can be passed a list of files instead of a single file.

OK.

> > +            ;; The .at files create shell scripts without shebangs. Erk.
> > +            (substitute* "tests/commands.at"
> > +                         (("./output.sh")
> > +                          (string-append (which "bash") " output.sh")))
>
> (which "sh") may be more correct (Bash behaves differently depending no
> whether it’s invoked as sh or bash.)

OK.

> Please align the opening parenthesis under the ‘u’ of ‘substitute*’.

OK.

Cyril.