[PATCH] gnu: Canonicalize paths before comparing.

  • Open
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Noé Lopez
Owner
unassigned
Submitted by
Noé Lopez
Severity
normal

Debbugs page

Noé Lopez wrote 1 months ago
(address . guix-patches@gnu.org)(name . Noé Lopez)(address . noe@xn--no-cja.eu)
9d65464d2049c6162c6ed4f022490eb59a38488a.1739021650.git.noelopez@free.fr
From: Noé Lopez <noelopez@free.fr>

The comparison would fail if the load path for guix was not already
canonicalized, since it is doing a string comparison.

* gnu/packages.scm (%patch-path): Canonicalize paths before
comparing.
* guix/ui.scm (try-canonicalize-path): Move to (guix utils).
* guix/utils.scm (try-canonicalize-path): New function.

Change-Id: Id5d51ce483af74ac4e122563d84cc3e8d78c3246
---
gnu/packages.scm | 11 ++++++-----
guix/ui.scm | 14 --------------
guix/utils.scm | 15 +++++++++++++++
3 files changed, 21 insertions(+), 19 deletions(-)

Toggle diff (84 lines)
diff --git a/gnu/packages.scm b/gnu/packages.scm
index bdd5d21940..d043d0616d 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -167,11 +167,12 @@ (define %patch-path
;; Define it after '%package-module-path' so that '%load-path' contains user
;; directories, allowing patches in $GUIX_PACKAGE_PATH to be found.
(make-parameter
- (map (lambda (directory)
- (if (string=? directory %distro-root-directory)
- (string-append directory "/gnu/packages/patches")
- directory))
- %load-path)))
+ (let ((root (try-canonicalize-path %distro-root-directory)))
+ (map (lambda (directory)
+ (if (string=? (try-canonicalize-path directory) root)
+ (string-append directory "/gnu/packages/patches")
+ directory))
+ %load-path))))
;; This procedure is used by Emacs-Guix up to 0.5.1.1, so keep it for now.
;; See <https://github.com/alezost/guix.el/issues/30>.
diff --git a/guix/ui.scm b/guix/ui.scm
index 87a448bf72..a3a9bf4e42 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -206,20 +206,6 @@ (define-syntax-rule (without-compiler-optimizations exp)
(parameterize (((@ (system base compile) default-optimization-level) 1))
exp))
-(define (try-canonicalize-path file)
- "Like 'canonicalize-path', but return FILE as-is if 'canonicalize-path'
-throws.
-
-This is necessary for corner cases where 'canonicalize-path' fails. One
-example is on Linux when a /dev/fd/N file denotes a pipe, represented as a
-symlink to a non-existent file like 'pipe:[1234]', as in this example:
-
- sh -c 'stat $(readlink -f /dev/fd/1)' | cat"
- (catch 'system-error
- (lambda ()
- (canonicalize-path file))
- (const file)))
-
(define* (load* file user-module
#:key (on-error 'nothing-special))
"Load the user provided Scheme source code FILE."
diff --git a/guix/utils.scm b/guix/utils.scm
index b6cf5aea4f..6e5b6b6caf 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -162,6 +162,7 @@ (define-module (guix utils)
compressed-output-port
call-with-compressed-output-port
canonical-newline-port
+ try-canonicalize-path
string-distance
string-closest
@@ -1150,6 +1151,20 @@ (define (canonical-newline-port port)
get-position
set-position!
close))
+
+(define (try-canonicalize-path file)
+ "Like 'canonicalize-path', but return FILE as-is if 'canonicalize-path'
+throws.
+
+This is necessary for corner cases where 'canonicalize-path' fails. One
+example is on Linux when a /dev/fd/N file denotes a pipe, represented as a
+symlink to a non-existent file like 'pipe:[1234]', as in this example:
+
+ sh -c 'stat $(readlink -f /dev/fd/1)' | cat"
+ (catch 'system-error
+ (lambda ()
+ (canonicalize-path file))
+ (const file)))
;;;
;;; Source location.

base-commit: 9c36d38614079611aebe4721b9e087f98e57b1b3
--
2.48.1
Ludovic Courtès wrote 1 months ago
(name . Noé Lopez)(address . noe@xn--no-cja.eu)(name . Josselin Poiret)(address . dev@jpoiret.xyz)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(name . Mathieu Othacehe)(address . othacehe@gnu.org)(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 76143@debbugs.gnu.org)(name . Christopher Baines)(address . guix@cbaines.net)
87pljrs6fc.fsf@gnu.org
Hi,

Noé Lopez <noe@noé.eu> skribis:

Toggle quote (18 lines)
> --- a/gnu/packages.scm
> +++ b/gnu/packages.scm
> @@ -167,11 +167,12 @@ (define %patch-path
> ;; Define it after '%package-module-path' so that '%load-path' contains user
> ;; directories, allowing patches in $GUIX_PACKAGE_PATH to be found.
> (make-parameter
> - (map (lambda (directory)
> - (if (string=? directory %distro-root-directory)
> - (string-append directory "/gnu/packages/patches")
> - directory))
> - %load-path)))
> + (let ((root (try-canonicalize-path %distro-root-directory)))
> + (map (lambda (directory)
> + (if (string=? (try-canonicalize-path directory) root)
> + (string-append directory "/gnu/packages/patches")
> + directory))
> + %load-path))))

I’m not sure what the goal is but please keep in mind that
‘canonicalize-path’ is expensive in terms of system calls (especially if
‘%load-path’ is long, and we’d pay it for all program startup times),
and that the comparison here remains brittle (checking the dev/ino
fields of ‘stat’ would be more accurate).

Thanks,
Ludo’.
Noé Lopez wrote 1 months ago
[PATCH v2] gnu: Find patches directory through symlinks.
(address . 76143@debbugs.gnu.org)(name . Noé Lopez)(address . noe@xn--no-cja.eu)
19bb17d762c3cddbd57c9f500f17e0b1ce957d36.1739714805.git.noelopez@free.fr
From: Noé Lopez <noelopez@free.fr>

This fixes a bug where patches would not be found in %patch-path when the
Guile load path would contain a different path (via symlink or trailing slash)
to the %distro-root-directory than what was previously found. We use stat to
make sure that two different paths to the same directory are still matched.

For example: if the Guile path was /guix/ and %distro-root-directory was
/guix, patches would not be found even though the two directories are the
same.

* gnu/packages.scm (%patch-path): Compare directories with directory=?.
* guix/utils.scm (directory=?): New procedure.
* tests/utils.scm: Add tests for directory=?.

Change-Id: I73f65b6c050cdeff85637e13ffd0319dcc1d4958
---
gnu/packages.scm | 2 +-
guix/utils.scm | 15 +++++++++++++++
tests/utils.scm | 13 +++++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)

Toggle diff (88 lines)
diff --git a/gnu/packages.scm b/gnu/packages.scm
index bdd5d21940..5cad0d50ff 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -168,7 +168,7 @@ (define %patch-path
;; directories, allowing patches in $GUIX_PACKAGE_PATH to be found.
(make-parameter
(map (lambda (directory)
- (if (string=? directory %distro-root-directory)
+ (if (directory=? directory %distro-root-directory)
(string-append directory "/gnu/packages/patches")
directory))
%load-path)))
diff --git a/guix/utils.scm b/guix/utils.scm
index b6cf5aea4f..a2537b4285 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -21,6 +21,7 @@
;;; Copyright © 2023 Zheng Junjie <873216071@qq.com>
;;; Copyright © 2023 Foundation Devices, Inc. <hello@foundationdevices.com>
;;; Copyright © 2024 Herman Rimm <herman@rimm.ee>
+;;; Copyright © 2025 Noé Lopez <noelopez@free.fr>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -162,6 +163,7 @@ (define-module (guix utils)
compressed-output-port
call-with-compressed-output-port
canonical-newline-port
+ directory=?
string-distance
string-closest
@@ -1150,6 +1152,19 @@ (define (canonical-newline-port port)
get-position
set-position!
close))
+
+(define* (directory=? directory #:rest directories)
+ (define (dev+ino directory)
+ (and-let* ((stats (stat directory #f))
+ (dev (stat:dev stats))
+ (ino (stat:ino stats)))
+ (cons dev ino)))
+ (define check (dev+ino directory))
+ (and check
+ (fold (lambda (element acc)
+ (and acc (equal? (dev+ino element) check)))
+ #t
+ directories)))
;;;
;;; Source location.
diff --git a/tests/utils.scm b/tests/utils.scm
index 462e43e2b1..88a88eba1d 100644
--- a/tests/utils.scm
+++ b/tests/utils.scm
@@ -6,6 +6,7 @@
;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2023 Foundation Devices, Inc. <hello@foundationdevices.com>
;;; Copyright © 2024 Herman Rimm <herman@rimm.ee>
+;;; Copyright © 2025 Noé Lopez <noelopez@free.fr>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -370,6 +371,18 @@ (define-public package-2\n 'package)\n"
;; way.
"avr32" "avr32-unknown-none")))
+;; Try to not depend on the user’s filesystem.
+(test-equal "directory=?"
+ '(#t #t #t #t #t #f #f)
+ (list
+ (directory=? "/" "/")
+ (directory=? "/../" "//")
+ (directory=? "//../" "/")
+ (directory=? "/")
+ (directory=? "/" "/../" "//" "//..//../")
+ (directory=? "/proc/99999999" "/proc/99999999") ;nonexistent directories
+ (directory=? "/proc/99999999/../../" "/")))
+
(test-end)
(false-if-exception (delete-file temp-file))

base-commit: 73d74032d580212e7b59644d3324677926e4339b
--
2.48.1
Noé Lopez wrote 1 months ago
Re: [bug#76143] [PATCH] gnu: Canonicalize paths before comparing.
(name . Ludovic Courtès)(address . ludo@gnu.org)(name . Josselin Poiret)(address . dev@jpoiret.xyz)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(name . Mathieu Othacehe)(address . othacehe@gnu.org)(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 76143@debbugs.gnu.org)(name . Christopher Baines)(address . guix@cbaines.net)
871pvy6knf.fsf@xn--no-cja.eu
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (31 lines)
> Hi,
>
> Noé Lopez <noe@noé.eu> skribis:
>
>> --- a/gnu/packages.scm
>> +++ b/gnu/packages.scm
>> @@ -167,11 +167,12 @@ (define %patch-path
>> ;; Define it after '%package-module-path' so that '%load-path' contains user
>> ;; directories, allowing patches in $GUIX_PACKAGE_PATH to be found.
>> (make-parameter
>> - (map (lambda (directory)
>> - (if (string=? directory %distro-root-directory)
>> - (string-append directory "/gnu/packages/patches")
>> - directory))
>> - %load-path)))
>> + (let ((root (try-canonicalize-path %distro-root-directory)))
>> + (map (lambda (directory)
>> + (if (string=? (try-canonicalize-path directory) root)
>> + (string-append directory "/gnu/packages/patches")
>> + directory))
>> + %load-path))))
>
> I’m not sure what the goal is but please keep in mind that
> ‘canonicalize-path’ is expensive in terms of system calls (especially if
> ‘%load-path’ is long, and we’d pay it for all program startup times),
> and that the comparison here remains brittle (checking the dev/ino
> fields of ‘stat’ would be more accurate).
>
> Thanks,
> Ludo’.

I was not aware that you could do that with stat, I’ve sent a v2 that
uses stat for comparison which works much better.

The problem I faced was that I needed to create environment variables by
hand so that I could load guile code (including guix’s) in a C program
with no external environment (the packagekit daemon). It would fail
because of not finding patches since I had a trailing slash in my path.

I’ve resolved the issue by removing the slash, but I wanted to fix it
for everyone else trying to use Guix as a library.
Noé Lopez wrote 1 months ago
[PATCH v3 0/1] gnu: Find patches directory through symlinks.
(address . 76143@debbugs.gnu.org)(name . Noé Lopez)(address . noelopez@free.fr)
cover.1739716173.git.noelopez@free.fr
From: Noé Lopez <noelopez@free.fr>

I sent the patch too early, sorry for the spam.

If you want to test this patch, you can try the following (changing the paths
to your checkout):

noe@lignux ~$ export GUILE_LOAD_PATH=/home/noe/src///guix-patch-review///
noe@lignux ~$ export GUILE_LOAD_COMPILED_PATH=/home/noe/src/guix-patch-review////
noe@lignux ~$ guile
[...]

Enter `,help' for help.
scheme@(guile-user)> ,use (gnu packages)
scheme@(guile-user)> (%patch-path)
$2 = ("/home/noe/src///guix-patch-review////gnu/packages/patches" "/gnu/store/ylwk2vn18dkzkj0nxq2h4vjzhz17bm7c-guile-3.0.9/share/guile/3.0" "/gnu/store/ylwk2vn18dkzkj0nxq2h4vjzhz17bm7c-guile-3.0.9/share/guile/site/3.0" "/gnu/store/ylwk2vn18dkzkj0nxq2h4vjzhz17bm7c-guile-3.0.9/share/guile/site" "/gnu/store/ylwk2vn18dkzkj0nxq2h4vjzhz17bm7c-guile-3.0.9/share/guile")

Check that /gnu/packages/patches was correctly added.

Noé Lopez (1):
gnu: Find patches directory through symlinks.

gnu/packages.scm | 2 +-
guix/utils.scm | 16 ++++++++++++++++
tests/utils.scm | 13 +++++++++++++
3 files changed, 30 insertions(+), 1 deletion(-)


base-commit: 73d74032d580212e7b59644d3324677926e4339b
--
2.48.1
Noé Lopez wrote 1 months ago
[PATCH v3 1/1] gnu: Find patches directory through symlinks.
(address . 76143@debbugs.gnu.org)(name . Noé Lopez)(address . noe@xn--no-cja.eu)
e8634b69fc06e5655dc0cc5789560869f0a69eca.1739716173.git.noelopez@free.fr
From: Noé Lopez <noelopez@free.fr>

This fixes a bug where patches would not be found in %patch-path when the
Guile load path would contain a different path (via symlink or trailing slash)
to the %distro-root-directory than what was previously found. We use stat to
make sure that two different paths to the same directory are still matched.

For example: if the Guile path was /guix/ and %distro-root-directory was
/guix, patches would not be found even though the two directories are the
same.

* gnu/packages.scm (%patch-path): Compare directories with directory=?.
* guix/utils.scm (directory=?): New procedure.
* tests/utils.scm: Add tests for directory=?.

Change-Id: I73f65b6c050cdeff85637e13ffd0319dcc1d4958
---
gnu/packages.scm | 2 +-
guix/utils.scm | 16 ++++++++++++++++
tests/utils.scm | 13 +++++++++++++
3 files changed, 30 insertions(+), 1 deletion(-)

Toggle diff (94 lines)
diff --git a/gnu/packages.scm b/gnu/packages.scm
index bdd5d21940..5cad0d50ff 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -168,7 +168,7 @@ (define %patch-path
;; directories, allowing patches in $GUIX_PACKAGE_PATH to be found.
(make-parameter
(map (lambda (directory)
- (if (string=? directory %distro-root-directory)
+ (if (directory=? directory %distro-root-directory)
(string-append directory "/gnu/packages/patches")
directory))
%load-path)))
diff --git a/guix/utils.scm b/guix/utils.scm
index b6cf5aea4f..7979eba040 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -21,6 +21,7 @@
;;; Copyright © 2023 Zheng Junjie <873216071@qq.com>
;;; Copyright © 2023 Foundation Devices, Inc. <hello@foundationdevices.com>
;;; Copyright © 2024 Herman Rimm <herman@rimm.ee>
+;;; Copyright © 2025 Noé Lopez <noelopez@free.fr>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -41,6 +42,7 @@ (define-module (guix utils)
#:use-module (guix config)
#:autoload (guix read-print) (object->string*)
#:use-module (srfi srfi-1)
+ #:use-module (srfi srfi-2)
#:use-module (srfi srfi-11)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-71)
@@ -162,6 +164,7 @@ (define-module (guix utils)
compressed-output-port
call-with-compressed-output-port
canonical-newline-port
+ directory=?
string-distance
string-closest
@@ -1150,6 +1153,19 @@ (define (canonical-newline-port port)
get-position
set-position!
close))
+
+(define* (directory=? directory #:rest directories)
+ (define (dev+ino directory)
+ (and-let* ((stats (stat directory #f))
+ (dev (stat:dev stats))
+ (ino (stat:ino stats)))
+ (cons dev ino)))
+ (define check (dev+ino directory))
+ (and check
+ (fold (lambda (element acc)
+ (and acc (equal? (dev+ino element) check)))
+ #t
+ directories)))
;;;
;;; Source location.
diff --git a/tests/utils.scm b/tests/utils.scm
index 462e43e2b1..88a88eba1d 100644
--- a/tests/utils.scm
+++ b/tests/utils.scm
@@ -6,6 +6,7 @@
;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2023 Foundation Devices, Inc. <hello@foundationdevices.com>
;;; Copyright © 2024 Herman Rimm <herman@rimm.ee>
+;;; Copyright © 2025 Noé Lopez <noelopez@free.fr>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -370,6 +371,18 @@ (define-public package-2\n 'package)\n"
;; way.
"avr32" "avr32-unknown-none")))
+;; Try to not depend on the user’s filesystem.
+(test-equal "directory=?"
+ '(#t #t #t #t #t #f #f)
+ (list
+ (directory=? "/" "/")
+ (directory=? "/../" "//")
+ (directory=? "//../" "/")
+ (directory=? "/")
+ (directory=? "/" "/../" "//" "//..//../")
+ (directory=? "/proc/99999999" "/proc/99999999") ;nonexistent directories
+ (directory=? "/proc/99999999/../../" "/")))
+
(test-end)
(false-if-exception (delete-file temp-file))
--
2.48.1
Ludovic Courtès wrote 4 weeks ago
Re: [bug#76143] [PATCH] gnu: Canonicalize paths before comparing.
(name . Noé Lopez)(address . noe@xn--no-cja.eu)(name . Josselin Poiret)(address . dev@jpoiret.xyz)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(name . Mathieu Othacehe)(address . othacehe@gnu.org)(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 76143@debbugs.gnu.org)(name . Christopher Baines)(address . guix@cbaines.net)
87o6yvbntm.fsf@gnu.org
Hi,

Noé Lopez <noe@noé.eu> skribis:

Toggle quote (5 lines)
> The problem I faced was that I needed to create environment variables by
> hand so that I could load guile code (including guix’s) in a C program
> with no external environment (the packagekit daemon). It would fail
> because of not finding patches since I had a trailing slash in my path.

I don’t fully understand the situation. My suggestion (but perhaps
you’re already doing that) would be for PackageKit to invoke ‘guix
repl’, not ‘guile’. The raison d’être of ‘guix repl’ was precisely to
have the load path for Guix and all its dependencies properly set up.

Ludo’.
Noé Lopez wrote 4 weeks ago
(name . Ludovic Courtès)(address . ludo@gnu.org)(name . Josselin Poiret)(address . dev@jpoiret.xyz)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(name . Mathieu Othacehe)(address . othacehe@gnu.org)(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 76143@debbugs.gnu.org)(name . Christopher Baines)(address . guix@cbaines.net)
87eczr2tkg.fsf@xn--no-cja.eu
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (15 lines)
> Hi,
>
> Noé Lopez <noe@noé.eu> skribis:
>
>> The problem I faced was that I needed to create environment variables by
>> hand so that I could load guile code (including guix’s) in a C program
>> with no external environment (the packagekit daemon). It would fail
>> because of not finding patches since I had a trailing slash in my path.
>
> I don’t fully understand the situation. My suggestion (but perhaps
> you’re already doing that) would be for PackageKit to invoke ‘guix
> repl’, not ‘guile’. The raison d’être of ‘guix repl’ was precisely to
> have the load path for Guix and all its dependencies properly set up.
>

Let me try to explain the situation better:

The Guix channel has a special case for where it stores its patches, so
guix needs to detect whether a directory is the Guix source to append
"/gnu/packages/patches" to it:

gnu/packages.scm(166)
(define %patch-path
(make-parameter
(map (lambda (directory)
(if (string=? directory %distro-root-directory)
(string-append directory "/gnu/packages/patches")
directory))
%load-path)))

The comparison is done with string=?, which is a very weak way of
comparing directories, as you know. But the directory list is supplied
by the environment, so its up to the user to set the path exactly equal
to %distro-root-directory.

Obviously, this can fail in many ways if the user is not using guix
through pre-inst-env or guix repl. My patch fixes that by comparing
with stat, making sure all paths to the guix source work.

For PackageKit specifically, I am using guix through guile’s C api and
needed to set the environment by hand, I fixed it by removing the
trailing slash.

Is that clearer?

All the best,
Noé
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 76143@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 76143
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help