* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
@ 2021-03-16 2:56 Philip McGrath
2021-03-16 5:43 ` Jack Hill
2021-04-10 20:59 ` [bug#47180] " Ludovic Courtès
0 siblings, 2 replies; 12+ messages in thread
From: Philip McGrath @ 2021-03-16 2:56 UTC (permalink / raw)
To: 47180; +Cc: jackhill, Philip McGrath
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.
Fixes https://issues.guix.gnu.org/47064
* gnu/packages/patches/racket-sh-via-rktio.patch: New file.
Adds a special case at the C level, controlled by a preprocessor macro,
to handle attempts to execute "/bin/sh".
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/scheme.scm (racket)[source](patches): Apply it.
(racket)[arguments](#:configure-flags): Add the CPP flag to enable it.
(racket)[arguments](#:modules): Use srfi-1.
(racket)[arguments](#:phases): Remove 'patch-/bin/sh and 'pre-configure.
Change 'pre-configure-minimal to just change directory.
Add 'patch-config.rktd-lib-search-dirs after 'build and before 'install
to configure Racket's "lib-search-dirs".
(racket, racket-minimal)[inputs]: Add bash-minimal as an explicit input.
(racket-minimal)[source]: Adjust to inherit patches from racket.
---
gnu/local.mk | 2 +
.../patches/racket-sh-via-rktio.patch | 91 +++++++++
gnu/packages/scheme.scm | 190 ++++++++----------
3 files changed, 179 insertions(+), 104 deletions(-)
create mode 100644 gnu/packages/patches/racket-sh-via-rktio.patch
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)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 12+ messages in thread* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-03-16 2:56 [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files Philip McGrath
@ 2021-03-16 5:43 ` Jack Hill
2021-03-16 17:37 ` Philip McGrath
2021-04-10 20:59 ` [bug#47180] " Ludovic Courtès
1 sibling, 1 reply; 12+ messages in thread
From: Jack Hill @ 2021-03-16 5:43 UTC (permalink / raw)
To: Philip McGrath; +Cc: 47180
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-03-16 5:43 ` Jack Hill
@ 2021-03-16 17:37 ` Philip McGrath
2021-03-17 3:20 ` Jack Hill
0 siblings, 1 reply; 12+ messages in thread
From: Philip McGrath @ 2021-03-16 17:37 UTC (permalink / raw)
To: Jack Hill; +Cc: 47180
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))
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-03-16 17:37 ` Philip McGrath
@ 2021-03-17 3:20 ` Jack Hill
2021-03-19 2:34 ` [bug#47180] [PATCH v2] " Philip McGrath
0 siblings, 1 reply; 12+ messages in thread
From: Jack Hill @ 2021-03-17 3:20 UTC (permalink / raw)
To: Philip McGrath; +Cc: 47180
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH v2] gnu: racket: Don't inject store paths into Racket files.
2021-03-17 3:20 ` Jack Hill
@ 2021-03-19 2:34 ` Philip McGrath
2021-04-12 16:48 ` bug#47180: [PATCH] " Ludovic Courtès
0 siblings, 1 reply; 12+ messages in thread
From: Philip McGrath @ 2021-03-19 2:34 UTC (permalink / raw)
To: 47180; +Cc: Philip McGrath
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.
Fixes https://issues.guix.gnu.org/47064
* gnu/packages/patches/racket-sh-via-rktio.patch: New file.
Adds a special case at the C level, controlled by a preprocessor macro,
to handle attempts to execute "/bin/sh".
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/scheme.scm (racket)[source](patches): Apply it.
(racket)[arguments](#:configure-flags): Add the CPP flag to enable it.
(racket)[arguments](#:modules): Use srfi-1.
(racket)[arguments](#:phases): Remove 'patch-/bin/sh and 'pre-configure.
Change 'pre-configure-minimal to just change directory.
Add 'patch-config.rktd-lib-search-dirs after 'build and before 'install
to configure Racket's "lib-search-dirs".
(racket, racket-minimal)[inputs]: Add bash-minimal as an explicit input.
(racket-minimal)[source]: Adjust to inherit patches from racket.
(racket-minimal)[arguments]: Inherit from racket: changes no longer needed.
---
gnu/local.mk | 2 +
.../patches/racket-sh-via-rktio.patch | 87 ++++++++
gnu/packages/scheme.scm | 191 ++++++++----------
3 files changed, 176 insertions(+), 104 deletions(-)
create mode 100644 gnu/packages/patches/racket-sh-via-rktio.patch
diff --git a/gnu/local.mk b/gnu/local.mk
index afd9c17f9c..1128dbd080 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..b4fefd1514
--- /dev/null
+++ b/gnu/packages/patches/racket-sh-via-rktio.patch
@@ -0,0 +1,87 @@
+From 3574b567c486d264d680a37586436c3b5a8cb978 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. 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 | 21 ++++++++++++++++++++-
+ 1 file changed, 20 insertions(+), 1 deletion(-)
+
+diff --git a/src/rktio/rktio_process.c b/src/rktio/rktio_process.c
+index 89202436c0..465ebdd5c5 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,23 @@ 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, 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.21.1 (Apple Git-122.3)
+
diff --git a/gnu/packages/scheme.scm b/gnu/packages/scheme.scm
index 10be0aa28a..b5d526bfc3 100644
--- a/gnu/packages/scheme.scm
+++ b/gnu/packages/scheme.scm
@@ -14,6 +14,7 @@
;;; Copyright © 2020 Pierre Neidhardt <mail@ambrevar.xyz>
;;; Copyright © 2020 Brett Gilio <brettg@gnu.org>
;;; Copyright © 2020 Edouard Klein <edk@beaver-labs.com>
+;;; Copyright © 2021 Philip McGrath <philip@philipmcgrath.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -43,6 +44,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 +413,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 +460,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 +562,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)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 12+ messages in thread* bug#47180: [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-03-19 2:34 ` [bug#47180] [PATCH v2] " Philip McGrath
@ 2021-04-12 16:48 ` Ludovic Courtès
0 siblings, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2021-04-12 16:48 UTC (permalink / raw)
To: Philip McGrath; +Cc: 47180-done
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?)
> 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.
>
> Fixes https://issues.guix.gnu.org/47064
>
> * gnu/packages/patches/racket-sh-via-rktio.patch: New file.
> Adds a special case at the C level, controlled by a preprocessor macro,
> to handle attempts to execute "/bin/sh".
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/scheme.scm (racket)[source](patches): Apply it.
> (racket)[arguments](#:configure-flags): Add the CPP flag to enable it.
> (racket)[arguments](#:modules): Use srfi-1.
> (racket)[arguments](#:phases): Remove 'patch-/bin/sh and 'pre-configure.
> Change 'pre-configure-minimal to just change directory.
> Add 'patch-config.rktd-lib-search-dirs after 'build and before 'install
> to configure Racket's "lib-search-dirs".
> (racket, racket-minimal)[inputs]: Add bash-minimal as an explicit input.
> (racket-minimal)[source]: Adjust to inherit patches from racket.
> (racket-minimal)[arguments]: Inherit from racket: changes no longer needed.
> ---
> gnu/local.mk | 2 +
> .../patches/racket-sh-via-rktio.patch | 87 ++++++++
> gnu/packages/scheme.scm | 191 ++++++++----------
> 3 files changed, 176 insertions(+), 104 deletions(-)
> create mode 100644 gnu/packages/patches/racket-sh-via-rktio.patch
Pushed as 834aa48504a24f0c79e858fc295edbf63815a408.
Thanks!
Ludo’.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-03-16 2:56 [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files Philip McGrath
2021-03-16 5:43 ` Jack Hill
@ 2021-04-10 20:59 ` Ludovic Courtès
2021-04-12 3:40 ` Philip McGrath
1 sibling, 1 reply; 12+ messages in thread
From: Ludovic Courtès @ 2021-04-10 20:59 UTC (permalink / raw)
To: Philip McGrath; +Cc: jackhill, 47180
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’.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-04-10 20:59 ` [bug#47180] " Ludovic Courtès
@ 2021-04-12 3:40 ` Philip McGrath
2021-04-12 12:55 ` Ludovic Courtès
2021-04-12 12:55 ` Ludovic Courtès
0 siblings, 2 replies; 12+ messages in thread
From: Philip McGrath @ 2021-04-12 3:40 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: jackhill, 47180
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-04-12 3:40 ` Philip McGrath
@ 2021-04-12 12:55 ` Ludovic Courtès
2021-04-12 12:55 ` Ludovic Courtès
1 sibling, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2021-04-12 12:55 UTC (permalink / raw)
To: Philip McGrath; +Cc: jackhill, 47180
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’.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-04-12 3:40 ` Philip McGrath
2021-04-12 12:55 ` Ludovic Courtès
@ 2021-04-12 12:55 ` Ludovic Courtès
2021-04-16 20:16 ` Philip McGrath
1 sibling, 1 reply; 12+ messages in thread
From: Ludovic Courtès @ 2021-04-12 12:55 UTC (permalink / raw)
To: Philip McGrath; +Cc: jackhill, 47180
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’.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-04-12 12:55 ` Ludovic Courtès
@ 2021-04-16 20:16 ` Philip McGrath
2021-04-17 10:12 ` Ludovic Courtès
0 siblings, 1 reply; 12+ messages in thread
From: Philip McGrath @ 2021-04-16 20:16 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: jackhill, 47180
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files.
2021-04-16 20:16 ` Philip McGrath
@ 2021-04-17 10:12 ` Ludovic Courtès
0 siblings, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2021-04-17 10:12 UTC (permalink / raw)
To: Philip McGrath; +Cc: jackhill, 47180
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’.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-04-17 10:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-16 2:56 [bug#47180] [PATCH] gnu: racket: Don't inject store paths into Racket files Philip McGrath
2021-03-16 5:43 ` Jack Hill
2021-03-16 17:37 ` Philip McGrath
2021-03-17 3:20 ` Jack Hill
2021-03-19 2:34 ` [bug#47180] [PATCH v2] " Philip McGrath
2021-04-12 16:48 ` bug#47180: [PATCH] " Ludovic Courtès
2021-04-10 20:59 ` [bug#47180] " Ludovic Courtès
2021-04-12 3:40 ` Philip McGrath
2021-04-12 12:55 ` Ludovic Courtès
2021-04-12 12:55 ` Ludovic Courtès
2021-04-16 20:16 ` Philip McGrath
2021-04-17 10:12 ` Ludovic Courtès
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).