[bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.

[Jack Hill <jackhill@jackhill.us>]

[-- Attachment #1: Type: text/plain, Size: 4604 bytes --]

On Tue, 16 Mar 2021, Philip McGrath wrote:

> Hi,
>
> On 3/16/21 1:43 AM, Jack Hill wrote:
>> I've tested applying and building this patch and it works!
>
> Glad to hear it!
>
>> (Now that DrRacket starts it appears that I may have a font issue with it. 
>> I'll investigate more an open a separate bug if so.)
>
> I realized to my chagrin when I saw your report that I hadn't actually used 
> any graphical programs via Guix, which of course would be an important thing 
> to test! I'll experiment, too, but yes, please do look for further issues.

I've opened #47204 for the fonts issue. I haven't done any investigation 
yet, and racket certainly wouldn't be the only package to run across fonts 
issues with Guix :)

[0] https://issues.guix.gnu.org/47204

I'm happy to help test things, that's what's nice about working on a 
community project.

> A good motivation for `guix import racket` is that we'll be able to run the 
> test suite, which lives in the `racket-test` package.

Indeed, and many more reasons.

>> `git am` complained about the following, but I can't spot them looking at 
>> the patch.
>
> I'm not sure what's up there … I generated it with `git send-email`. But I'm 
> not very familiar with this workflow, so it's quite possible there's 
> something I should have done/be doing differently.

I had assumed it was actually a problem with adding inadvertent whitespace 
to the patch rather than how it was sent. However, since I couldn't 
actually find the extra spaces anywhere, I'm not too worried about it. 
Other than the warnings it applied cleanly.

>>> +    3. "/bin/sh" does not exist (if it does, we don't want to change
>>> +       the expected behavior); and
>> 
>> Do we really want #3? That would have a different effect than the current 
>> patching behavior intends. /bin/sh is present by default on Guix System, 
>> and likely to be present of foreign distros. I think in generally we like 
>> to avoid this dynamic binding and instead use the sh that was included as a 
>> package input.
>
> That's a very good question.
>
> The non-obvious scenario I've been considering is that DrRacket's 
> `Racket|Create Executable…`, `raco distribute`, or the function 
> `assemble-distribution` from the `compiler/distribute` library can compile 
> your Racket program and bundle it with its runtime support into a tarball, 
> which is intended to be portable enough that you can email it to your friend 
> running some other GNU/Linux distribution and they can run it, too. One 
> implication of this feature is that a well-behaved Racket library should 
> cooperate with the compilation manager to register any foreign shared 
> libraries it may want to bring along.
>
> I don't use this feature much and it would take further testing before I'd be 
> confident that works properly on Guix, but it was definitely broken with the 
> old/current way of patching Racket source files with paths to foreign 
> libraries on the store. Configuring the `lib-search-dirs` is at least a step 
> closer to The Right Thing.
>
> This patch doesn't try to bring along "sh", and I don't think it should: the 
> relevant consideration here is that `librktio` will be bundled (IIRC as part 
> of `libracketcs.a`), so whatever version of `rktio_process` we compile ought 
> to work without Guix.
>
> That said, I think you may be right that, on Guix, using the sh that was a 
> package input may be less surprising than considering "/bin/sh" if it exists. 
> (I also think it's pretty unreasonable to put something other than a 
> POSIX-compatable shell at "/bin/sh" and expect any good to come of it.) All 
> the Racket docs[1] promise is that the relevant function "executes a shell 
> command asynchronously (using sh on Unix and Mac OS, cmd on Windows)", which 
> I take to mean that our obligation is to provide a sh, not for it to be any 
> particular sh or a user-configurable sh. (It does not consult SHELL, for 
> example.)
>
> So, if this still seems right to you, I propose to revise the patch by 
> dropping condition #3: then we'll still fall back to "/bin/sh" if the 
> built-in path doesn't exist, as presumably will be the case if we're being 
> executed in a non-Guix environment, but we'll unconditionally use the sh that 
> was a package input on Guix.
>
> Thanks for looking over this thoughtfully!
>
> -Philip
>
> [1]: 
> https://docs.racket-lang.org/reference/subprocess.html#(def._((lib._racket%2Fsystem..rkt)._process))

I hadn't considered raco distribute (I guess I'm spoiled by `guix pack`). 
Your proposed change addresses my concern.

Thanks!
Jack