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

[Jack Hill <jackhill@jackhill.us>]

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

Wow, that was speedy. Thanks for the care you've taken thinking about these 
issues. I've tested applying and building this patch and it works!

(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 don't think I know enough about Guix or Racket internals to give it a 
proper review or judge whether this will be robust against future grafting 
problems, but I do have a few comments


`git am` complained about the following, but I can't spot them looking at 
the patch.

.git/rebase-apply/patch:83: trailing whitespace.

.git/rebase-apply/patch:100: trailing whitespace.

.git/rebase-apply/patch:122: trailing whitespace.
-- 
.git/rebase-apply/patch:124: new blank line at EOF.


On Mon, 15 Mar 2021, Philip McGrath wrote:

> Apparently, during grafting, Guix can somehow mangle compiled
> Racket CS files (.zo) such that Racket will refuse to load them.
> (Maybe it has something to do with compression?)
> So, we stop patching Racket sources with absolute paths to store
> files (i.e. for foreign libraries to dlopen).
> Instead, we put them in a data file that doesn't get compiled or,
> in one case, embed it in C.

[…]

> +Guix should enable the special case by defining the C preprocessor
> +macro `GUIX_RKTIO_PATCH_BIN_SH` with the path to `sh` in the store.
> +If:
> +
> +    1. The `GUIX_RKTIO_PATCH_BIN_SH` macro is defined; and
> +
> +    2. `rktio_process` is called with the exact path "/bin/sh"; and
> +
> +    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.

> +    4. The path specified by `GUIX_RKTIO_PATCH_BIN_SH` does exists;
> +
> +then `rktio_process` will execute the file specified
> +by `GUIX_RKTIO_PATCH_BIN_SH` instead of "/bin/sh".
> +
> +Compared to previous attempts to patch the Racket sources,
> +making this change at the C level is both:
> +
> +    - More comprehensive: it catches all attempts to execute "/bin/sh",
> +      without having to track down the source of every occurance; and
> +
> +    - Less intrusive: by guarding the special case with a C preprocessor
> +      conditional and a runtime check that the file in the store exists,
> +      we make it much less likely that it will "leak" out of Guix.

Again, I might be wrong by preferring to always avoid /bin/sh even when 
it's available. Hopefully someone more knowledgeable will come along.

Best,
Jack