| Message ID | 20210316025632.9767-1-philip@philipmcgrath.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#47180] gnu: racket: Don't inject store paths into Racket files. | expand |
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.
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. 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. > `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. >> + 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))
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
Hi Philip, Philip McGrath <philip@philipmcgrath.com> skribis: > 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?) If those files are compressed, and if a store file name survives despite compression, then grafting can patch it, which could lead to checksum mismatches or similar. What error message does Racket produce? > 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. That solves the problem for Racket itself, but wouln’t Racket libraries have the same issue? Would it be an option to instead turn off compression and keep doing things as usual? Thanks for looking into it, and sorry for the delay! Ludo’.
Hi! On 4/10/21 4:59 PM, Ludovic Courtès wrote: > Hi Philip, > > Philip McGrath <philip@philipmcgrath.com> skribis: > >> 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?) > > If those files are compressed, and if a store file name survives despite > compression, then grafting can patch it, which could lead to checksum > mismatches or similar. Yes, that's what seems to be happening. > > What error message does Racket produce? The first error I heard of (and reproduced) was reported by Jack in <https://issues.guix.gnu.org/47064>: ``` $bytevector-uncompress: internal error uncompressing #"\0\0\0\0chez\310\224\206:\r()#\201\256R-d\205\233\24\363\5\20\201P\6A\v\300\0\16\f\6\31\2\f\6\f&H\275\0\1\0\362\bA\377e\0\1\0C\6A\21\3\v\300\0\201\265!\f\6\n\0\a\1\35\0\1+\0\360\27\201\375\300\0\0\0\17\205\210Z\0\0M\215\245\b\4\0\0M9fH\17\206fZ\0\0I\2... context...: body of "/gnu/store/mmrax3f1vx3c8ih9hhgffpvka6chk96w-racket-8.0/share/racket/pkgs/gui-lib/mred/private/wx/gtk/utils.rkt" body of "/gnu/store/mmrax3f1vx3c8ih9hhgffpvka6chk96w-racket-8.0/share/racket/pkgs/gui-lib/mred/private/wx/platform.rkt" ``` The error message is referring to an internal Chez Scheme primitive. The report at <https://issues.guix.gnu.org/47614> seems to me to be an alternative manifestation of the same problem: the hexdump there is useful and explains why I did see some of the store path when I had attempted to investigate using `strings`. So there seem to be at least three bad cases: 1. The grafter can mangle .zo files so that Racket can't read them at all. 2. The grafter can miss store references in .zo files, so Racket could end up using the ungrafted versions. 3. With a garbage collection, Racket could try to use the ungrafted versions but fail to find them at runtime. >> 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. > > That solves the problem for Racket itself, but wouln’t Racket libraries > have the same issue? Because the problems are triggered by grafting, they only affect packages that have been patched by Guix. For now, Guix doesn't have the ability to install more Racket packages. In the longer term, the one-sentence answer is that it's always possible we might find and switch to a better approach, but this seems most promising. (A bit more of my current thinking on that toward the end.) > Would it be an option to instead turn off compression and keep doing > things as usual? In theory, this should be possible. I see two significant downsides: 1. Compiled code would be much larger—maybe twice as big—and, if I recall correctly, load times would be worse, too. With the move to Racket CS, existing Racket code moved from a world of small and cheap bytecode to a world of machine code: the default compression settings have been tuned to avoid an unacceptable worsening of binary size and load time. 2. Racket very intentionally does not specify the format of zo files, and indeed the details routinely change: I think there have now been 14 such changes on Git since the 8.0 release in February. Continuing to patch zo files sets us up for future breakage, and it seems like it would be especially easy for maintainers of the Guix package to miss the implications of such changes to low-level implementation details (as I did!). For example, Chez Scheme seems to make compression options programmer-configurable even within a single object file: if Racket exposed such options, we could well end up here again. More broadly, I think the best strategy for Racket packaging will be, as much as possible, to use Racket's supported configuration features rather than Guix-specific hacks. This seems especially viable since Racket has been willing to accept unobtrusive patches upstream that help things go smoothly for Guix, e.g. with 8.1 we should no longer need any patches to the build scripts: we're all friendly, parentheses-loving folks. For another example, it looks like existing "racket-store-checksum-override.patch" fixes a previous issue discussed in <https://issues.guix.gnu.org/30680> caused by grafting zo files: I hope this change will also let us remove that patch, though I'd want to test more before proposing we drop it. So these problems aren't fundamentally new; they just have an additional symptom since the change to Racket CS by default in Racket 8.0. If we can fix the root problem of violating Racket's assumptions by patching zo files, we should be able to stop hunting down symptoms. Rather than using "config.rktd", an alternative approach would be to set things up so that `dlopen` would find the foreign libraries, perhaps via `LD_LIBRARY_PATH`. This has some intriguing possibilities: I could imagine Guix providing an alternate `dlopen` implementation that might be useful beyond Racket. Nix apparently configures some things via `LD_LIBRARY_PATH`, but their approach (as I understand it) relies on generating Racket scripts around all Racket-generated executable, which causes other problems. There should be workarounds, but it seems better to avoid going down that road if we can. Finally, here's a sketch of how `guix import racket` and such might work. Racket's package system has a concept of "package scope", so that "installation" scope can coexist with narrower scopes (mostly per-user scope, though there are more complex possibilities). Right now, Guix puts installation scope in the read-only store, which basically corresponds to how other package managers put it in root-owned places, except Guix can't write to the store to install additional packages. I'm still at the information-gathering stage, but my current thinking is that the hypothetical `racket-build-system` should basically take the source package and turn it into what Racket calls a "built" package ready to be installed in `static-link` mode, which includes compiling the code and building the docs (which can involve quite a lot, e.g. ray tracing icons). Then a profile hook could knit together all of the Racket packages into an installation package scope. For packages that depend on foreign libraries, this would be a chance for Guix to add the necessary paths to the "config.rktd" for the installation. -Philip
Hi Philip, Philip McGrath <philip@philipmcgrath.com> skribis: > So there seem to be at least three bad cases: > > 1. The grafter can mangle .zo files so that Racket can't > read them at all. > 2. The grafter can miss store references in .zo files, so Racket could > end up using the ungrafted versions. > 3. With a garbage collection, Racket could try to use the ungrafted > versions but fail to find them at runtime. Yes, similar to the problems observed with SBCL: https://issues.guix.gnu.org/33848 >> Would it be an option to instead turn off compression and keep doing >> things as usual? > > In theory, this should be possible. I see two significant downsides: > > 1. Compiled code would be much larger—maybe twice as big—and, if I > recall correctly, load times would be worse, too. With the move to > Racket CS, existing Racket code moved from a world of small and > cheap bytecode to a world of machine code: the default compression > settings have been tuned to avoid an unacceptable worsening of > binary size and load time. Interesting (I’m curious how load time can be improved by (1) reading files in memory instead of merely mmap’ing, and (2) decompressing.) [...] > More broadly, I think the best strategy for Racket packaging will be, > as much as possible, to use Racket's supported configuration features > rather than Guix-specific hacks. This seems especially viable since > Racket has been willing to accept unobtrusive patches upstream that > help things go smoothly for Guix, e.g. with 8.1 we should no longer > need any patches to the build scripts: we're all friendly, > parentheses-loving folks. That makes sense to me, I’m all for uniting with parentheses-loving folks of the world. :-) > For another example, it looks like existing > "racket-store-checksum-override.patch" fixes a previous issue > discussed in <https://issues.guix.gnu.org/30680> caused by grafting zo > files: I hope this change will also let us remove that patch, though > I'd want to test more before proposing we drop it. So these problems > aren't fundamentally new; they just have an additional symptom since > the change to Racket CS by default in Racket 8.0. If we can fix the > root problem of violating Racket's assumptions by patching zo files, > we should be able to stop hunting down symptoms. OK. I’m testing v2 of the patch and will push shortly if everything goes well. > Rather than using "config.rktd", an alternative approach would be to > set things up so that `dlopen` would find the foreign libraries, > perhaps via `LD_LIBRARY_PATH`. This has some intriguing possibilities: > I could imagine Guix providing an alternate `dlopen` implementation > that might be useful beyond Racket. What would that alternate dlopen do? It still has to know where to look for things, somehow. > Nix apparently configures some things via `LD_LIBRARY_PATH`, but their > approach (as I understand it) relies on generating Racket scripts > around all Racket-generated executable, which causes other > problems. There should be workarounds, but it seems better to avoid > going down that road if we can. Yeah, as a rule of thumb I’d say: don’t fiddle with LD_LIBRARY_PATH. > Finally, here's a sketch of how `guix import racket` and such might > work. Racket's package system has a concept of "package scope", so > that "installation" scope can coexist with narrower scopes (mostly > per-user scope, though there are more complex possibilities). Right > now, Guix puts installation scope in the read-only store, which > basically corresponds to how other package managers put it in > root-owned places, except Guix can't write to the store to install > additional packages. I'm still at the information-gathering stage, but > my current thinking is that the hypothetical `racket-build-system` > should basically take the source package and turn it into what Racket > calls a "built" package ready to be installed in `static-link` mode, > which includes compiling the code and building the docs (which can > involve quite a lot, e.g. ray tracing icons). Then a profile hook > could knit together all of the Racket packages into an installation > package scope. For packages that depend on foreign libraries, this > would be a chance for Guix to add the necessary paths to the > "config.rktd" for the installation. I’m not sure I follow the details and I’m glad you have a plan. Finally having a Racket importer would be sweet! Thanks! Ludo’.
Hi Philip, Philip McGrath <philip@philipmcgrath.com> skribis: > So there seem to be at least three bad cases: > > 1. The grafter can mangle .zo files so that Racket can't > read them at all. > 2. The grafter can miss store references in .zo files, so Racket could > end up using the ungrafted versions. > 3. With a garbage collection, Racket could try to use the ungrafted > versions but fail to find them at runtime. Yes, similar to the problems observed with SBCL: https://issues.guix.gnu.org/33848 >> Would it be an option to instead turn off compression and keep doing >> things as usual? > > In theory, this should be possible. I see two significant downsides: > > 1. Compiled code would be much larger—maybe twice as big—and, if I > recall correctly, load times would be worse, too. With the move to > Racket CS, existing Racket code moved from a world of small and > cheap bytecode to a world of machine code: the default compression > settings have been tuned to avoid an unacceptable worsening of > binary size and load time. Interesting (I’m curious how load time can be improved by (1) reading files in memory instead of merely mmap’ing, and (2) decompressing.) [...] > More broadly, I think the best strategy for Racket packaging will be, > as much as possible, to use Racket's supported configuration features > rather than Guix-specific hacks. This seems especially viable since > Racket has been willing to accept unobtrusive patches upstream that > help things go smoothly for Guix, e.g. with 8.1 we should no longer > need any patches to the build scripts: we're all friendly, > parentheses-loving folks. That makes sense to me, I’m all for uniting with parentheses-loving folks of the world. :-) > For another example, it looks like existing > "racket-store-checksum-override.patch" fixes a previous issue > discussed in <https://issues.guix.gnu.org/30680> caused by grafting zo > files: I hope this change will also let us remove that patch, though > I'd want to test more before proposing we drop it. So these problems > aren't fundamentally new; they just have an additional symptom since > the change to Racket CS by default in Racket 8.0. If we can fix the > root problem of violating Racket's assumptions by patching zo files, > we should be able to stop hunting down symptoms. OK. I’m testing v2 of the patch and will push shortly if everything goes well. > Rather than using "config.rktd", an alternative approach would be to > set things up so that `dlopen` would find the foreign libraries, > perhaps via `LD_LIBRARY_PATH`. This has some intriguing possibilities: > I could imagine Guix providing an alternate `dlopen` implementation > that might be useful beyond Racket. What would that alternate dlopen do? It still has to know where to look for things, somehow. > Nix apparently configures some things via `LD_LIBRARY_PATH`, but their > approach (as I understand it) relies on generating Racket scripts > around all Racket-generated executable, which causes other > problems. There should be workarounds, but it seems better to avoid > going down that road if we can. Yeah, as a rule of thumb I’d say: don’t fiddle with LD_LIBRARY_PATH. > Finally, here's a sketch of how `guix import racket` and such might > work. Racket's package system has a concept of "package scope", so > that "installation" scope can coexist with narrower scopes (mostly > per-user scope, though there are more complex possibilities). Right > now, Guix puts installation scope in the read-only store, which > basically corresponds to how other package managers put it in > root-owned places, except Guix can't write to the store to install > additional packages. I'm still at the information-gathering stage, but > my current thinking is that the hypothetical `racket-build-system` > should basically take the source package and turn it into what Racket > calls a "built" package ready to be installed in `static-link` mode, > which includes compiling the code and building the docs (which can > involve quite a lot, e.g. ray tracing icons). Then a profile hook > could knit together all of the Racket packages into an installation > package scope. For packages that depend on foreign libraries, this > would be a chance for Guix to add the necessary paths to the > "config.rktd" for the installation. I’m not sure I follow the details and I’m glad you have a plan. Finally having a Racket importer would be sweet! Thanks! Ludo’.
Hi, On 4/12/21 8:55 AM, Ludovic Courtès wrote: > Philip McGrath <philip@philipmcgrath.com> skribis: >> Rather than using "config.rktd", an alternative approach would be to >> set things up so that `dlopen` would find the foreign libraries, >> perhaps via `LD_LIBRARY_PATH`. This has some intriguing possibilities: >> I could imagine Guix providing an alternate `dlopen` implementation >> that might be useful beyond Racket. > > What would that alternate dlopen do? It still has to know where to look > for things, somehow. This was a very fuzzy thought with a lot of reliance on "somehow"—I'm not certain it would actually make sense or even be possible—but what I had in mind is that `dlopen`, together with the dynamic linker and its various configuration and cache files, has some places it will search for shared libraries, e.g. in "/lib" and "/usr/lib". If `gnu-build-system` could somehow communicate with those mechanisms, then packages doing things like `dlopen("libm.so.6", RTLD_LAZY)` wouldn't need to have their source code rewritten to include store files: Guix would arrange for `dlopen` to find "libm.so.6" among the package inputs. Then Guix would only need to know how to graft whatever mechanism it used to configure things for `dlopen`, rather than having to worry about all of the strange things compilers might do with string literals. But I don't have much of an idea of what "somehow" might be, and I'm not really a C hacker when I can avoid it :) -Philip
Hi, Philip McGrath <philip@philipmcgrath.com> skribis: > On 4/12/21 8:55 AM, Ludovic Courtès wrote: >> Philip McGrath <philip@philipmcgrath.com> skribis: >>> Rather than using "config.rktd", an alternative approach would be to >>> set things up so that `dlopen` would find the foreign libraries, >>> perhaps via `LD_LIBRARY_PATH`. This has some intriguing possibilities: >>> I could imagine Guix providing an alternate `dlopen` implementation >>> that might be useful beyond Racket. >> What would that alternate dlopen do? It still has to know where to >> look >> for things, somehow. > > This was a very fuzzy thought with a lot of reliance on "somehow"—I'm > not certain it would actually make sense or even be possible—but what > I had in mind is that `dlopen`, together with the dynamic linker and > its various configuration and cache files, has some places it will > search for shared libraries, e.g. in "/lib" and "/usr/lib". If > `gnu-build-system` could somehow communicate with those mechanisms, > then packages doing things like `dlopen("libm.so.6", RTLD_LAZY)` > wouldn't need to have their source code rewritten to include store > files: Guix would arrange for `dlopen` to find "libm.so.6" among the > package inputs. Then Guix would only need to know how to graft > whatever mechanism it used to configure things for `dlopen`, rather > than having to worry about all of the strange things compilers might > do with string literals. To me, the most practical way it can work is by explicitly replacing “libfoo.so” with “/gnu/store/…/libfoo.so” in the source. We could change the ‘dlopen’ implementation altogether so that it (say) looks for a meta-data file listing search paths, but that’s the kind of intrusive change I’d rather avoid. Also, there are cases where we actually want “dynamic binding” (search for the lib and load it *if* it’s available) rather than “static binding” (hard-code the name of the library so it’s always found when we dlopen it). This is the case for plugins and such. I guess we need to explore the problem space a bit further. :-) Thanks, Ludo’.
diff --git a/gnu/local.mk b/gnu/local.mk index cf8849cf59..51dcf4e6e6 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -40,6 +40,7 @@ # Copyright © 2020 Malte Frank Gerdes <mate.f.gerdes@gmail.com> # Copyright © 2020 Vinicius Monego <monego@posteo.net> # Copyright © 2021 Björn Höfling <bjoern.hoefling@bjoernhoefling.de> +# Copyright © 2021 Philip McGrath <philip@philipmcgrath.com> # # This file is part of GNU Guix. # @@ -1629,6 +1630,7 @@ dist_patch_DATA = \ %D%/packages/patches/ripperx-missing-file.patch \ %D%/packages/patches/rpcbind-CVE-2017-8779.patch \ %D%/packages/patches/rtags-separate-rct.patch \ + %D%/packages/patches/racket-sh-via-rktio.patch \ %D%/packages/patches/racket-store-checksum-override.patch \ %D%/packages/patches/remake-impure-dirs.patch \ %D%/packages/patches/retroarch-LIBRETRO_DIRECTORY.patch \ diff --git a/gnu/packages/patches/racket-sh-via-rktio.patch b/gnu/packages/patches/racket-sh-via-rktio.patch new file mode 100644 index 0000000000..870e8eb5f2 --- /dev/null +++ b/gnu/packages/patches/racket-sh-via-rktio.patch @@ -0,0 +1,91 @@ +From 34fd01842d5211e73260721b7a516487e982497c Mon Sep 17 00:00:00 2001 +From: Philip McGrath <philip@philipmcgrath.com> +Date: Thu, 4 Mar 2021 04:11:50 -0500 +Subject: [PATCH] patch rktio_process for "/bin/sh" on Guix + +Racket provides the functions `system` and `process`, +which execute shell commands using `sh` (or `cmd` on Windows). +Racket assumes that `sh` can be found at "/bin/sh", +which is not necessarily true on Guix. + +This patch adds a special case for "/bin/sh" to `rktio_process`, +the C function that implements the core of `system`, `process`, +and related Racket functions. + +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 + + 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. +--- + src/rktio/rktio_process.c | 22 +++++++++++++++++++++- + 1 file changed, 21 insertions(+), 1 deletion(-) + +diff --git a/src/rktio/rktio_process.c b/src/rktio/rktio_process.c +index 89202436c0..ff65a434e2 100644 +--- a/src/rktio/rktio_process.c ++++ b/src/rktio/rktio_process.c +@@ -1224,12 +1224,14 @@ int rktio_process_allowed_flags(rktio_t *rktio) + /*========================================================================*/ + + rktio_process_result_t *rktio_process(rktio_t *rktio, +- const char *command, int argc, rktio_const_string_t *argv, ++ /* PATCHED for Guix (next line) */ ++ const char *_guix_orig_command, int argc, rktio_const_string_t *argv, + rktio_fd_t *stdout_fd, rktio_fd_t *stdin_fd, rktio_fd_t *stderr_fd, + rktio_process_t *group_proc, + const char *current_directory, rktio_envvars_t *envvars, + int flags) + { ++ const char *command; /* PATCHED for Guix */ + rktio_process_result_t *result; + intptr_t to_subprocess[2], from_subprocess[2], err_subprocess[2]; + int pid; +@@ -1255,6 +1257,24 @@ rktio_process_result_t *rktio_process(rktio_t *rktio, + int i; + #endif + ++/* BEGIN PATCH for Guix */ ++#if defined(GUIX_RKTIO_PATCH_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: */ ++ command = ++ ((0 == strcmp(_guix_orig_command, "/bin/sh")) ++ && (! rktio_file_exists(rktio, "/bin/sh")) ++ && rktio_file_exists(rktio, GUIX_AS_a_STR(GUIX_RKTIO_PATCH_BIN_SH))) ++ ? GUIX_AS_a_STR(GUIX_RKTIO_PATCH_BIN_SH) ++ : _guix_orig_command; ++# undef GUIX_AS_a_STR ++# undef GUIX_AS_a_STR_HELPER ++#else ++ command = _guix_orig_command; ++#endif ++/* END PATCH for Guix */ ++ + /* avoid compiler warnings: */ + to_subprocess[0] = -1; + to_subprocess[1] = -1; +-- +2.25.1 + diff --git a/gnu/packages/scheme.scm b/gnu/packages/scheme.scm index 10be0aa28a..8656fef1ab 100644 --- a/gnu/packages/scheme.scm +++ b/gnu/packages/scheme.scm @@ -43,6 +43,7 @@ #:use-module (guix build-system trivial) #:use-module (gnu packages autotools) #:use-module (gnu packages bdw-gc) + #:use-module (gnu packages bash) #:use-module (gnu packages compression) #:use-module (gnu packages databases) #:use-module (gnu packages libevent) @@ -411,94 +412,26 @@ implementation techniques and as an expository tool.") (base32 "047wpjblfzmf1msz7snrp2c2h0zxyzlmbsqr9bwsyvz3frcg0888")) (patches (search-patches + "racket-sh-via-rktio.patch" + ;; TODO: If we're no longer patching Racket source + ;; files with store paths, we may also fix the + ;; issue that necessitated the following patch: "racket-store-checksum-override.patch")))) (build-system gnu-build-system) (arguments - '(#:configure-flags - '("--enable-libz" + `(#:configure-flags + `(,(string-append "CPPFLAGS=-DGUIX_RKTIO_PATCH_BIN_SH=" + (assoc-ref %build-inputs "sh") + "/bin/sh") + "--enable-libz" "--enable-liblz4") + #:modules + ((guix build gnu-build-system) + (guix build utils) + (srfi srfi-1)) #:phases (modify-phases %standard-phases - (add-before 'configure 'pre-configure-minimal - (lambda* (#:key inputs #:allow-other-keys) - ;; Patch dynamically loaded libraries with their absolute paths. - (let* ((library-path (search-path-as-string->list - (getenv "LIBRARY_PATH"))) - (find-so (lambda (soname) - (search-path - library-path - (format #f "~a.so" soname))))) - (substitute* "collects/db/private/sqlite3/ffi.rkt" - (("ffi-lib sqlite-so") - (format #f "ffi-lib \"~a\"" (find-so "libsqlite3")))) - (substitute* "collects/openssl/libssl.rkt" - (("ffi-lib libssl-so") - (format #f "ffi-lib \"~a\"" (find-so "libssl")))) - (substitute* "collects/openssl/libcrypto.rkt" - (("ffi-lib libcrypto-so") - (format #f "ffi-lib \"~a\"" (find-so "libcrypto"))))) - (chdir "src") - #t)) - (add-before 'pre-configure-minimal 'pre-configure - (lambda* (#:key inputs #:allow-other-keys) - ;; Patch dynamically loaded libraries with their absolute paths. - (let* ((library-path (search-path-as-string->list - (getenv "LIBRARY_PATH"))) - (find-so (lambda (soname) - (search-path - library-path - (format #f "~a.so" soname)))) - (patch-ffi-libs (lambda (file libs) - (for-each - (lambda (lib) - (substitute* file - (((format #f "\"~a\"" lib)) - (format #f "\"~a\"" (find-so lib))))) - libs)))) - (substitute* "share/pkgs/math-lib/math/private/bigfloat/gmp.rkt" - (("ffi-lib libgmp-so") - (format #f "ffi-lib \"~a\"" (find-so "libgmp")))) - (substitute* "share/pkgs/math-lib/math/private/bigfloat/mpfr.rkt" - (("ffi-lib libmpfr-so") - (format #f "ffi-lib \"~a\"" (find-so "libmpfr")))) - (substitute* "share/pkgs/readline-lib/readline/rktrl.rkt" - (("\\(getenv \"PLT_READLINE_LIB\"\\)") - (format #f "\"~a\"" (find-so "libedit")))) - (for-each - (lambda (x) (apply patch-ffi-libs x)) - '(("share/pkgs/draw-lib/racket/draw/unsafe/cairo-lib.rkt" - ("libfontconfig" "libcairo")) - ("share/pkgs/draw-lib/racket/draw/unsafe/glib.rkt" - ("libglib-2.0" "libgmodule-2.0" "libgobject-2.0")) - ("share/pkgs/draw-lib/racket/draw/unsafe/jpeg.rkt" - ("libjpeg")) - ("share/pkgs/draw-lib/racket/draw/unsafe/pango.rkt" - ("libpango-1.0" "libpangocairo-1.0")) - ("share/pkgs/draw-lib/racket/draw/unsafe/png.rkt" - ("libpng")) - ("share/pkgs/db-lib/db/private/odbc/ffi.rkt" - ("libodbc")) - ("share/pkgs/gui-lib/mred/private/wx/gtk/x11.rkt" - ("libX11")) - ("share/pkgs/gui-lib/mred/private/wx/gtk/gsettings.rkt" - ("libgio-2.0")) - ("share/pkgs/gui-lib/mred/private/wx/gtk/gtk3.rkt" - ("libgdk-3" "libgtk-3")) - ("share/pkgs/gui-lib/mred/private/wx/gtk/unique.rkt" - ("libunique-1.0")) - ("share/pkgs/gui-lib/mred/private/wx/gtk/utils.rkt" - ("libgdk-x11-2.0" "libgdk_pixbuf-2.0" "libgtk-x11-2.0")) - ("share/pkgs/gui-lib/mred/private/wx/gtk/gl-context.rkt" - ("libGL")) - ("share/pkgs/sgl/gl.rkt" - ("libGL" "libGLU"))))) - #t)) - (add-after 'unpack 'patch-/bin/sh - (lambda _ - (substitute* "collects/racket/system.rkt" - (("/bin/sh") (which "sh"))) - #t)) - (add-after 'patch-/bin/sh 'patch-chez-configure + (add-after 'unpack 'patch-chez-configure (lambda* (#:key inputs outputs #:allow-other-keys) (substitute* "src/cs/c/Makefile.in" (("/bin/sh") (which "sh"))) @@ -526,12 +459,69 @@ implementation techniques and as an expository tool.") (("/bin/cp") (which "cp")) (("/bin/echo") (which "echo"))) (substitute* "makefiles/installsh" - (("/bin/true") (which "true"))))))) + (("/bin/true") (which "true")))) + #t)) + (add-before 'configure 'pre-configure-minimal + (lambda* (#:key inputs #:allow-other-keys) + (chdir "src") + #t)) + (add-after 'build 'patch-config.rktd-lib-search-dirs + (lambda* (#:key inputs outputs #:allow-other-keys) + ;; We do this between the `build` and `install` phases + ;; so that we have racket to read and write the hash table, + ;; but it comes before `raco setup`, when foreign libraries + ;; are needed to build the documentation. + (define out (assoc-ref outputs "out")) + (apply invoke + "./cs/c/racketcs" + "-e" + ,(format #f + "~s" + '(let* ((args + (vector->list + (current-command-line-arguments))) + (file (car args)) + (extra-lib-search-dirs (cdr args))) + (write-to-file + (hash-update + (file->value file) + 'lib-search-dirs + (lambda (dirs) + (append dirs extra-lib-search-dirs)) + null) + #:exists 'truncate/replace + file))) + "--" + "../etc/config.rktd" + (filter-map (lambda (lib) + (cond + ((assoc-ref inputs lib) + => (lambda (pth) + (string-append pth "/lib"))) + (else + #f))) + '("cairo" + "fontconfig" + "glib" + "glu" + "gmp" + "gtk+" + "libjpeg" + "libpng" + "libx11" + "mesa" + "mpfr" + "openssl" + "pango" + "sqlite" + "unixodbc" + "libedit"))) + #t))) ;; XXX: how to run them? #:tests? #f)) (inputs - `(;; Hardcode dynamically loaded libraries for better functionality. - ;; sqlite and libraries for `racket/draw' are needed to build the doc. + `(;; sqlite and libraries for `racket/draw' are needed to build the doc. + ("sh" ,bash-minimal) ("zlib" ,zlib) ("zlib:static" ,zlib "static") ("lz4" ,lz4) @@ -571,29 +561,21 @@ of languages such as Typed Racket, R5RS and R6RS Scheme, and Datalog.") (inherit racket) (name "racket-minimal") (version (package-version racket)) - (source (origin - (method url-fetch) - (uri (list (string-append "https://mirror.racket-lang.org/installers/" - version "/racket-minimal-src.tgz") - ;; this mirror seems to have broken HTTPS: - (string-append - "http://mirror.informatik.uni-tuebingen.de/mirror/racket/" - version "/racket-minimal-src.tgz"))) - (sha256 - (base32 - "0mwyffw4gcci8wmzxa3j28h03h0gsz55aard8qrk3lri8r2xyg21")) - (patches (search-patches - "racket-store-checksum-override.patch")))) + (source + (origin + (inherit (package-source racket)) + (uri (list (string-append "https://mirror.racket-lang.org/installers/" + version "/racket-minimal-src.tgz") + ;; this mirror seems to have broken HTTPS: + (string-append + "http://mirror.informatik.uni-tuebingen.de/mirror/racket/" + version "/racket-minimal-src.tgz"))) + (sha256 "0mwyffw4gcci8wmzxa3j28h03h0gsz55aard8qrk3lri8r2xyg21"))) (synopsis "Racket without bundled packages such as Dr. Racket") - (arguments - (substitute-keyword-arguments (package-arguments racket) - ((#:phases phases) - `(modify-phases ,phases - ;; Delete fix that applies to files not included in the minimal package. - (delete 'pre-configure))))) (inputs `(("openssl" ,openssl) ("sqlite" ,sqlite) + ("sh" ,bash-minimal) ("zlib" ,zlib) ("zlib:static" ,zlib "static") ("lz4" ,lz4)