Package: guix-patches;
Reported by: Josselin Poiret <dev <at> jpoiret.xyz>
Date: Sun, 8 May 2022 14:17:02 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55316 in the body.
You can then email your comments to 55316 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix-patches <at> gnu.org:bug#55316; Package guix-patches.
(Sun, 08 May 2022 14:17:02 GMT) Full text and rfc822 format available.Josselin Poiret <dev <at> jpoiret.xyz>:guix-patches <at> gnu.org.
(Sun, 08 May 2022 14:17:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Josselin Poiret <dev <at> jpoiret.xyz> To: guix-patches <at> gnu.org Cc: Josselin Poiret <dev <at> jpoiret.xyz> Subject: [PATCH] scripts: package: Transform before creating manifest entries. Date: Sun, 8 May 2022 16:16:06 +0200
* guix/scripts/package.scm (options->installable): Add TRANSFORM
argument, to be able to directly transform the new packages before
creating their manifest entries.
(process-actions): Remove transform-entry, and step3.
---
Hello everyone,
Someone on IRC [1] reported that they couldn't run
guix install emacs-avy emacs-embark --with-branch=emacs-avy=master --with-branch=emacs-embark=master
without guix complaining about conflicting entries of emacs-avy and
the propagated one from emacs-embark, even in a profile without either
of them. The issue was that package transformations were applied to
the contents of the generated manifest entries, but weren't applied to
any of their dependencies. To solve this, I figured it would be simpler
to just apply the transformation from the start, only creating the manifest
entries after they have been applied. Also, the
default `package->manifest-entry` preserves transformations properties by default,
so this doesn't lose any info either.
With this patch applied, the above command works as expected for me.
Best,
Josselin Poiret
[1] https://logs.guix.gnu.org/guix/2022-05-08.log#134728
guix/scripts/package.scm | 36 ++++++++++++------------------------
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index d007005607..4673cf18b2 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -10,6 +10,7 @@
;;; Copyright © 2020 Ricardo Wurmus <rekado <at> elephly.net>
;;; Copyright © 2020 Simon Tournier <zimon.toutoune <at> gmail.com>
;;; Copyright © 2018 Steve Sprang <scs <at> stevesprang.com>
+;;; Copyright © 2022 Josselin Poiret <dev <at> jpoiret.xyz>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -694,10 +695,10 @@ (define (package->manifest-entry* package output)
(manifest-entry-with-provenance
(package->manifest-entry package output)))
-(define (options->installable opts manifest transaction)
- "Given MANIFEST, the current manifest, and OPTS, the result of 'args-fold',
-return an variant of TRANSACTION that accounts for the specified installations
-and upgrades."
+(define (options->installable opts manifest transform transaction)
+ "Given MANIFEST, the current manifest, OPTS, and TRANSFORM, the result of
+'args-fold', return an variant of TRANSACTION that accounts for the specified
+installations, upgrades and transformations."
(define upgrade?
(options->upgrade-predicate opts))
@@ -714,13 +715,14 @@ (define to-install
(('install . (? package? p))
;; When given a package via `-e', install the first of its
;; outputs (XXX).
- (package->manifest-entry* p "out"))
+ (package->manifest-entry* (transform p) "out"))
(('install . (? string? spec))
(if (store-path? spec)
(store-item->manifest-entry spec)
(let-values (((package output)
(specification->package+output spec)))
- (package->manifest-entry* package output))))
+ (package->manifest-entry* (transform package)
+ output))))
(('install . obj)
(leave (G_ "cannot install non-package object: ~s~%")
obj))
@@ -979,16 +981,6 @@ (define allow-collisions? (assoc-ref opts 'allow-collisions?))
(define profile (or (assoc-ref opts 'profile) %current-profile))
(define transform (options->transformation opts))
- (define (transform-entry entry)
- (let ((item (transform (manifest-entry-item entry))))
- (manifest-entry-with-transformations
- (manifest-entry
- (inherit entry)
- (item item)
- (version (if (package? item)
- (package-version item)
- (manifest-entry-version entry)))))))
-
(when (equal? profile %current-profile)
;; Normally the daemon created %CURRENT-PROFILE when we connected, unless
;; it's a version that lacks the fix for <https://bugs.gnu.org/37744>
@@ -1021,16 +1013,12 @@ (define (transform-entry entry)
(map load-manifest files))))))
(step1 (options->removable opts manifest
(manifest-transaction)))
- (step2 (options->installable opts manifest step1))
- (step3 (manifest-transaction
- (inherit step2)
- (install (map transform-entry
- (manifest-transaction-install step2)))))
- (new (manifest-perform-transaction manifest step3))
+ (step2 (options->installable opts manifest transform step1))
+ (new (manifest-perform-transaction manifest step2))
(trans (if (null? files)
- step3
+ step2
(fold manifest-transaction-install-entry
- step3
+ step2
(manifest-entries manifest)))))
(warn-about-old-distro)
--
2.35.1
guix-patches <at> gnu.org:bug#55316; Package guix-patches.
(Mon, 09 May 2022 09:53:02 GMT) Full text and rfc822 format available.Message #8 received at 55316 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Josselin Poiret <dev <at> jpoiret.xyz> Cc: 55316 <at> debbugs.gnu.org Subject: Re: bug#55316: [PATCH] scripts: package: Transform before creating manifest entries. Date: Mon, 09 May 2022 11:51:58 +0200
Hello! Josselin Poiret <dev <at> jpoiret.xyz> skribis: > * guix/scripts/package.scm (options->installable): Add TRANSFORM > argument, to be able to directly transform the new packages before > creating their manifest entries. > (process-actions): Remove transform-entry, and step3. > --- > Hello everyone, > > Someone on IRC [1] reported that they couldn't run > > guix install emacs-avy emacs-embark --with-branch=emacs-avy=master --with-branch=emacs-embark=master > > without guix complaining about conflicting entries of emacs-avy and > the propagated one from emacs-embark, even in a profile without either > of them. The issue was that package transformations were applied to > the contents of the generated manifest entries, but weren't applied to > any of their dependencies. Interesting. > To solve this, I figured it would be simpler to just apply the > transformation from the start, only creating the manifest entries > after they have been applied. Also, the default > `package->manifest-entry` preserves transformations properties by > default, so this doesn't lose any info either. As discussed on IRC, could you make sure transformation properties are preserved in the manifest? I only see one test that explicitly checks that, "transaction-upgrade-entry, transformation options preserved" in tests/packages.scm, and then there’s "options->transformation + package->manifest-entry" in tests/transformations.scm. Also, could you add a test that reproduces the problem being fixed? Thanks! Ludo’.
guix-patches <at> gnu.org:bug#55316; Package guix-patches.
(Mon, 09 May 2022 14:55:01 GMT) Full text and rfc822 format available.Message #11 received at 55316 <at> debbugs.gnu.org (full text, mbox):
From: Josselin Poiret <dev <at> jpoiret.xyz> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, 55316 <at> debbugs.gnu.org Subject: [PATCH v2] scripts: package: Transform before creating manifest entries. Date: Mon, 9 May 2022 16:54:10 +0200
* guix/scripts/package.scm (options->installable): Add TRANSFORM
argument, to be able to directly transform the new packages before
creating their manifest entries.
(process-actions): Remove transform-entry, and step3, transforming
directly in step2.
* tests/guix-package.sh: Add test.
---
Hello Ludo,
Thanks for your review. As mentioned on IRC, those tests don't test
process-actions, which is where the logic resides. I added a test to
guix-package.sh that follows the original issue instead, note although
that between yesterday and today, emacs-consult has changed enough to
not compile anymore with `--with-branch=emacs-consult=main` (branch
rename notwithstanding).
Best,
Josselin
guix/scripts/package.scm | 36 ++++++++++++------------------------
tests/guix-package.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 24 deletions(-)
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index d007005607..4673cf18b2 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -10,6 +10,7 @@
;;; Copyright © 2020 Ricardo Wurmus <rekado <at> elephly.net>
;;; Copyright © 2020 Simon Tournier <zimon.toutoune <at> gmail.com>
;;; Copyright © 2018 Steve Sprang <scs <at> stevesprang.com>
+;;; Copyright © 2022 Josselin Poiret <dev <at> jpoiret.xyz>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -694,10 +695,10 @@ (define (package->manifest-entry* package output)
(manifest-entry-with-provenance
(package->manifest-entry package output)))
-(define (options->installable opts manifest transaction)
- "Given MANIFEST, the current manifest, and OPTS, the result of 'args-fold',
-return an variant of TRANSACTION that accounts for the specified installations
-and upgrades."
+(define (options->installable opts manifest transform transaction)
+ "Given MANIFEST, the current manifest, OPTS, and TRANSFORM, the result of
+'args-fold', return an variant of TRANSACTION that accounts for the specified
+installations, upgrades and transformations."
(define upgrade?
(options->upgrade-predicate opts))
@@ -714,13 +715,14 @@ (define to-install
(('install . (? package? p))
;; When given a package via `-e', install the first of its
;; outputs (XXX).
- (package->manifest-entry* p "out"))
+ (package->manifest-entry* (transform p) "out"))
(('install . (? string? spec))
(if (store-path? spec)
(store-item->manifest-entry spec)
(let-values (((package output)
(specification->package+output spec)))
- (package->manifest-entry* package output))))
+ (package->manifest-entry* (transform package)
+ output))))
(('install . obj)
(leave (G_ "cannot install non-package object: ~s~%")
obj))
@@ -979,16 +981,6 @@ (define allow-collisions? (assoc-ref opts 'allow-collisions?))
(define profile (or (assoc-ref opts 'profile) %current-profile))
(define transform (options->transformation opts))
- (define (transform-entry entry)
- (let ((item (transform (manifest-entry-item entry))))
- (manifest-entry-with-transformations
- (manifest-entry
- (inherit entry)
- (item item)
- (version (if (package? item)
- (package-version item)
- (manifest-entry-version entry)))))))
-
(when (equal? profile %current-profile)
;; Normally the daemon created %CURRENT-PROFILE when we connected, unless
;; it's a version that lacks the fix for <https://bugs.gnu.org/37744>
@@ -1021,16 +1013,12 @@ (define (transform-entry entry)
(map load-manifest files))))))
(step1 (options->removable opts manifest
(manifest-transaction)))
- (step2 (options->installable opts manifest step1))
- (step3 (manifest-transaction
- (inherit step2)
- (install (map transform-entry
- (manifest-transaction-install step2)))))
- (new (manifest-perform-transaction manifest step3))
+ (step2 (options->installable opts manifest transform step1))
+ (new (manifest-perform-transaction manifest step2))
(trans (if (null? files)
- step3
+ step2
(fold manifest-transaction-install-entry
- step3
+ step2
(manifest-entries manifest)))))
(warn-about-old-distro)
diff --git a/tests/guix-package.sh b/tests/guix-package.sh
index d1b383d2ad..c2f5e39de0 100644
--- a/tests/guix-package.sh
+++ b/tests/guix-package.sh
@@ -1,6 +1,7 @@
# GNU Guix --- Functional package management for GNU
# Copyright © 2012-2022 Ludovic Courtès <ludo <at> gnu.org>
# Copyright © 2013 Nikita Karetnikov <nikita <at> karetnikov.org>
+# Copyright © 2022 Josselin Poiret <dev <at> jpoiret.xyz>
#
# This file is part of GNU Guix.
#
@@ -210,6 +211,35 @@ test "$(readlink -f "$profile/bin/guile")" \
test ! -f "$profile/bin/sed"
rm "$profile" "$profile"-[0-9]-link
+# Make sure transformations apply to propagated inputs and don't lead to
+# conflicts when installing them alongside, see bug
+# <https://lists.gnu.org/archive/html/guix-patches/2022-05/msg00277.html>
+mkdir "$module_dir"
+cat > "$module_dir/test.scm" <<EOF
+(define-module (test)
+ #:use-module (guix packages)
+ #:use-module (gnu packages base)
+ #:use-module (guix build-system trivial))
+
+(define-public dummy-package
+ (package
+ (name "dummy-package")
+ (version "1")
+ (source #f)
+ (build-system trivial-build-system)
+ (propagated-inputs
+ (list hello))
+ (synopsis "dummy")
+ (description "dummy")
+ (home-page "dummy")
+ (license #f)))
+EOF
+guix package -p "$profile" -L "$module_dir"\
+ -i hello dummy-package \
+ --without-tests=hello -n
+rm "$module_dir/test.scm"
+rmdir "$module_dir"
+
# Profiles with a relative file name. Make sure we don't create dangling
# symlinks--see bug report at
# <https://lists.gnu.org/archive/html/guix-devel/2018-07/msg00036.html>.
--
2.36.0
Ludovic Courtès <ludo <at> gnu.org>:Josselin Poiret <dev <at> jpoiret.xyz>:Message #16 received at 55316-done <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Josselin Poiret <dev <at> jpoiret.xyz> Cc: 55316-done <at> debbugs.gnu.org Subject: Re: bug#55316: [PATCH] scripts: package: Transform before creating manifest entries. Date: Mon, 23 May 2022 15:15:41 +0200
[Message part 1 (text/plain, inline)]
Hi Josselin, Josselin Poiret <dev <at> jpoiret.xyz> skribis: > * guix/scripts/package.scm (options->installable): Add TRANSFORM > argument, to be able to directly transform the new packages before > creating their manifest entries. > (process-actions): Remove transform-entry, and step3, transforming > directly in step2. > * tests/guix-package.sh: Add test. Finally applied with the cosmetic change below, thank you! Ludo’.
[Message part 2 (text/x-patch, inline)]
diff --git a/tests/guix-package.sh b/tests/guix-package.sh index c2f5e39de0..dedba2fd74 100644 --- a/tests/guix-package.sh +++ b/tests/guix-package.sh @@ -212,8 +212,8 @@ test ! -f "$profile/bin/sed" rm "$profile" "$profile"-[0-9]-link # Make sure transformations apply to propagated inputs and don't lead to -# conflicts when installing them alongside, see bug -# <https://lists.gnu.org/archive/html/guix-patches/2022-05/msg00277.html> +# conflicts when installing them alongside, see +# <https://issues.guix.gnu.org/55316>. mkdir "$module_dir" cat > "$module_dir/test.scm" <<EOF (define-module (test)
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Tue, 21 Jun 2022 11:24:04 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.