Hi,

On Monday, August 8, 2022 4:53:42 AM EDT Liliana Marie Prikler wrote:
> Am Montag, dem 08.08.2022 um 02:10 -0400 schrieb Philip McGrath:
> > +diff --git a/c/prim5.c b/c/prim5.c
> > +index 5a07893..926d68d 100644
> > +--- a/c/prim5.c
> > ++++ b/c/prim5.c
> > +@@ -746,6 +746,22 @@ static ptr s_process(char *s, IBOOL stderrp) {
> > +
> > +     INT tofds[2], fromfds[2], errfds[2];
> > +     struct sigaction act, oint_act;
> > ++    /* BEGIN PATCH for Guix */
> > ++#if defined(GUIX_RKTIO_BIN_SH)
> > ++# define GUIX_AS_a_STR_HELPER(x) #x
> > ++# define GUIX_AS_a_STR(x) GUIX_AS_a_STR_HELPER(x)
> > ++    /* A level of indirection makes `#` work as needed: */
> > ++    struct stat guix_stat_buf;
> > ++    char *guix_sh =
> > ++      (0 == stat(GUIX_AS_a_STR(GUIX_RKTIO_BIN_SH), &guix_stat_buf))
> > ++      ? GUIX_AS_a_STR(GUIX_RKTIO_BIN_SH)
> > ++      : "/bin/sh";
> > ++# undef GUIX_AS_a_STR
> > ++# undef GUIX_AS_a_STR_HELPER
> > ++#else /* GUIX_RKTIO_BIN_SH */
> > ++    char *guix_sh = "/bin/sh";
> > ++#endif
> > ++    /* END PATCH for Guix */
> 
> /* BEGIN PATCH for Guix */ and /* END PATCH for Guix */ is in my humble
> opinion superfluous (though apparently also present in the already
> exsting patch, whose author might disagree).

My goal here originally was making it clear in `guix build --source` exactly 
what we'd changed from upstream. (That's less relevant for the patches with 
"backport" in the name, which have all been merged upstream.) It certainly is 
debatable, given that "guix" is mentioned in the variable/macro names, but, 
given that it's currently there, I'm not inclined to change the status quo.

> Also, I think this could easily be submitted upstream if you named it
> RKTIO_SHELL and rktio_shell respectively, with the default to
> "/bin/sh".  Then, we'd simply have to -DRKTIO_SHELL=/path/to/bin/sh in
> our #:make-flags.
> 

I'm not strongly opposed to sending these patches upstream. The main reason I 
haven't, and also the reason that, were I a Racket committer (which I'm not), 
I'd be slightly hesitant to merge them, is that I can't think of a 
circumstance other than Guix for which these patches would be useful: even Nix 
apparently provides "/bin/sh" and "/bin/env" in build containers.

If the patches are indeed specific to Guix, they haven't been a burden to 
maintain—this is the only time they've been changed at all—and keeping them 
ourselves gives us maximum flexibility to change them if we do want to for any 
reason.

In any case, even if we do upstream them, we'd still need the patches until 
8.7.

> 
> As for absorbing racket-specific patches into chez-scheme itself, I'm
> not too sure if I agree with that approach.  Maybe a different prefix
> rather than RKTIO should be used here – one that fits chez.
> 

I can understand the hesitation, but I think on balance these are not "Racket-
specific patches". First, just in case this wasn't clear, the issue affects both 
variants of Chez Scheme: see the end of this email for an example. More 
generally, any programming language that provides a way to run shell commands 
will need to deal with this issue: that includes libc's `system` function (I 
haven't looked at what we do there), and it also came up with SML/NJ: https://
lists.gnu.org/archive/html/help-guix/2021-11/msg00036.html

For the packages that are developed in the main Racket Git repository, I would 
be loathe to use more than one preprocessor macro for this purpose. While at 
the moment it works nicely for us to control all of the intermediate build 
steps explicitly as Guix packages, many build modes handle some of those steps 
automatically, and it would be very unpleasant to have to configure with three 
different flags, potentially in both CPPFLAGS and CPPFLAGS_FOR_BUILD.

The question, then, comes down to upstream Chez Scheme, and to me the benefits 
of using the same macro for these two very closely related projects far 
outweighs the downside. (For one metric of their interrelationship, over 10% 
of the commits to upstream Chez since it became free software in 2016 have 
come from Racket contributors—though this metric badly undercounts the c. 
375,000 lines of code developed by Dybvig et al. since 1984.)

I guess another possibility would be to call the macro something like 
GUIX_BIN_SH. That would make sense if we intended to adopt this approach more 
broadly, as I suggested in the email linked above about SML/NJ, but I think 
we'd need some consensus in that case to reserve  a concise name that wouldn't 
conflict with other uses in Guix.

-Philip

--8<---------------cut here---------------start------------->8---
philip@avalon:~$ guix shell --container chez-scheme -- scheme
Chez Scheme Version 9.5.8
Copyright 1984-2022 Cisco Systems, Inc.

> (get-string-all (car (process "echo $BASH")))
"/bin/sh\n"
> 
philip@avalon:~$ guix shell --container chez-scheme-for-racket -- scheme
Chez Scheme Version 9.5.7.6
Copyright 1984-2021 Cisco Systems, Inc.

> (get-string-all (car (process "echo $BASH")))
"/bin/sh\n"
> 
philip@avalon:~$
philip@avalon:~$ guix time-machine --url=https://gitlab.com/philip1/guix-patches --commit=5e5e8f491c7cbee3ef7a21437a52675dd47d186e --disable-
authentication -- shell --container chez-scheme -- scheme
Updating channel 'guix' from Git repository at 'https://gitlab.com/philip1/
guix-patches'...
guix time-machine: warning: channel authentication disabled
Computing Guix derivation for 'x86_64-linux'... -
Chez Scheme Version 9.5.8
Copyright 1984-2022 Cisco Systems, Inc.

> (get-string-all (car (process "echo $BASH")))
"/gnu/store/chfwin3a4qp1znnpsjbmydr2jbzk0d6y-bash-minimal-5.1.8/bin/sh\n"
> 
philip@avalon:~$ guix time-machine --url=https://gitlab.com/philip1/guix-patches --commit=5e5e8f491c7cbee3ef7a21437a52675dd47d186e --disable-
authentication -- shell --container chez-scheme-for-racket -- scheme
Updating channel 'guix' from Git repository at 'https://gitlab.com/philip1/
guix-patches'...
guix time-machine: warning: channel authentication disabled
Computing Guix derivation for 'x86_64-linux'... -
Chez Scheme Version 9.5.7.6
Copyright 1984-2021 Cisco Systems, Inc.

> (get-string-all (car (process "echo $BASH")))
"/gnu/store/chfwin3a4qp1znnpsjbmydr2jbzk0d6y-bash-minimal-5.1.8/bin/sh\n"
> 
--8<---------------cut here---------------end--------------->8---