From mboxrd@z Thu Jan  1 00:00:00 1970
From: Cyril Roelandt <tipecaml@gmail.com>
Subject: Re: [PATCH] Add tcsh.
Date: Mon, 04 Feb 2013 23:40:19 +0100
Message-ID: <511038D3.6020404@gmail.com>
References: <1360013226-16132-1-git-send-email-tipecaml@gmail.com>
	<87txprogl4.fsf@gnu.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Return-path: <bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([208.118.235.92]:57478)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tipecaml@gmail.com>) id 1U2Ut2-0004tq-QX
	for bug-guix@gnu.org; Mon, 04 Feb 2013 17:51:42 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <tipecaml@gmail.com>) id 1U2Ut1-0003oo-NF
	for bug-guix@gnu.org; Mon, 04 Feb 2013 17:51:40 -0500
In-Reply-To: <87txprogl4.fsf@gnu.org>
List-Id: Bug reports for GNU Guix <bug-guix.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/bug-guix>,
	<mailto:bug-guix-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/bug-guix>
List-Post: <mailto:bug-guix@gnu.org>
List-Help: <mailto:bug-guix-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/bug-guix>,
	<mailto:bug-guix-request@gnu.org?subject=subscribe>
Errors-To: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org
Sender: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org
To: =?UTF-8?B?THVkb3ZpYyBDb3VydMOocw==?= <ludo@gnu.org>
Cc: bug-guix@gnu.org

On 02/04/2013 11:32 PM, Ludovic Courtès wrote:
> Cyril Roelandt<tipecaml@gmail.com>  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.