* bug#65225: ‘guix shell --system=ALIEN’ builds for both systems @ 2023-08-11 11:10 Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-06 0:00 ` bug#65225: [PATCH] environment: Build the profile for the requested system Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-11 11:43 ` bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix 0 siblings, 2 replies; 7+ messages in thread From: Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-11 11:10 UTC (permalink / raw) To: 65225 Hi, ekaitz noticed this when building a mes-derived package that fails on x86_64. Here's a reproducer that uses upstream Guix: $ guix shell --system=riscv64-linux drawterm --no-grafts --rebuild-cache --dry-run | grep drv$ /gnu/store/4c02fgswkbldys93w4vgj0gwax2ly4bh-profile.drv $ guix build --dry-run /gnu/store/4c02fgswkbldys93w4vgj0gwax2ly4bh-profile.drv | grep drawterm /gnu/store/mkxyhjq117wdalc0gbz4468blqxih1kn-drawterm-20210628-1.c97fe46 /gnu/store/h8dk35aw2n8rj1hki6dkdrknfly26vq8-drawterm-20210628-1.c97fe46 /gnu/store/fl4iimlcdnlkarjm8m6z3392wss6b8yr-drawterm-20210628-1.c97fe46.drv → /gnu/store/mkxyhjq117wdalc0gbz4468blqxih1kn-drawterm-20210628-1.c97fe46 is riscv64-linux. /gnu/store/9a1pji59hzacrmpb65nk3pp3m01niqf3-drawterm-20210628-1.c97fe46.drv → /gnu/store/h8dk35aw2n8rj1hki6dkdrknfly26vq8-drawterm-20210628-1.c97fe46 is x86_64-linux. Eventually the ‘correct’ riscv64 drawterm ends up in the shell, but Guix should never have built the x86_64 version. Kind regards, T G-R Sent from a Web browser. Excuse or enjoy my brevity. ^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#65225: [PATCH] environment: Build the profile for the requested system. 2023-08-11 11:10 bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-06 0:00 ` Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-12 10:33 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-11 11:43 ` bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix 1 sibling, 1 reply; 7+ messages in thread From: Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-06 0:00 UTC (permalink / raw) To: 65225 Cc: Christopher Baines, Josselin Poiret, Ludovic Courtès, Mathieu Othacehe, Ricardo Wurmus, Simon Tournier, Tobias Geerinckx-Rice Previously, ‘--system=’ did not affect profile hooks, meaning that all packages would be built for both the host and requested systems. * guix/scripts/environment.scm (guix-environment*): Parameterize %current-system to match the requested ‘--system=’. Reported by ekaitz in #guix. --- guix/scripts/environment.scm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm index 9712389842..27f7e53549 100644 --- a/guix/scripts/environment.scm +++ b/guix/scripts/environment.scm @@ -1146,7 +1146,8 @@ (define (guix-environment* opts) (warning (G_ "no packages specified; creating an empty environment~%"))) ;; Use the bootstrap Guile when requested. - (parameterize ((%graft? (assoc-ref opts 'graft?)) + (parameterize ((%current-system system) + (%graft? (assoc-ref opts 'graft?)) (%guile-for-build (and store-needed? (package-derivation base-commit: 9e71d4fd6b3893ae87cb079b57d7a8fe6e9e7914 -- 2.41.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* bug#65225: [PATCH] environment: Build the profile for the requested system. 2023-08-06 0:00 ` bug#65225: [PATCH] environment: Build the profile for the requested system Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-12 10:33 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-12 10:42 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-12 20:51 ` Ludovic Courtès 0 siblings, 2 replies; 7+ messages in thread From: Josselin Poiret via Bug reports for GNU Guix @ 2023-08-12 10:33 UTC (permalink / raw) To: Tobias Geerinckx-Rice, 65225 Cc: Tobias Geerinckx-Rice, Simon Tournier, Mathieu Othacehe, Ludovic Courtès, Christopher Baines, Ricardo Wurmus [-- Attachment #1: Type: text/plain, Size: 5051 bytes --] Hi Tobias, Tobias Geerinckx-Rice <me@tobias.gr> writes: > Previously, ‘--system=’ did not affect profile hooks, meaning that all > packages would be built for both the host and requested systems. > > * guix/scripts/environment.scm (guix-environment*): Parameterize > %current-system to match the requested ‘--system=’. > > Reported by ekaitz in #guix. > --- > guix/scripts/environment.scm | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm > index 9712389842..27f7e53549 100644 > --- a/guix/scripts/environment.scm > +++ b/guix/scripts/environment.scm > @@ -1146,7 +1146,8 @@ (define (guix-environment* opts) > (warning (G_ "no packages specified; creating an empty environment~%"))) > > ;; Use the bootstrap Guile when requested. > - (parameterize ((%graft? (assoc-ref opts 'graft?)) > + (parameterize ((%current-system system) > + (%graft? (assoc-ref opts 'graft?)) > (%guile-for-build > (and store-needed? > (package-derivation > > base-commit: 9e71d4fd6b3893ae87cb079b57d7a8fe6e9e7914 > -- > 2.41.0 So, I've looked into this deeper because this fix didn't seem satisfying to me: it suggests that the implementation of profile-derivation itself is wrong, and I wanted to fix it instead. Here's what I uncovered: %current-system (applies mutatis mutandis to %current-target-system) is a Guile parameter. That means that it is accessed through a function call, and its values really depends on where that function call occurs. Now, this interacts with the store monad in a cumbersome way: monadic values in this case are functions (lambda (store) ...) returning two values, the actual output and the store. These functions are run only at the run-with-store call. Now, there are two non-equivalent ways of getting the current system in a monadic context. You can either do (mlet ((system -> (%current-system)) ...) or (mlet ((system (current-system)) ...) The former directly evaluates (%current-system), while the latter only evaluates (%current-system) when the monadic value is run! What does this mean for our case here? Well, the problem lies with how the hooks are lowered: they use (gexp->derivation ...) without the optional #:system keyword. This looks up the current system at call time with the mlet -> construct, so everything should be okay, right? Well, the hooks are run through a mapm/accumulate-builds call, which puts everything in a monadic value, effectively delaying the look-up until monadic run time. --8<---------------cut here---------------start------------->8--- (mlet* %store-monad (... (extras (if (null? (manifest-entries manifest)) (return '()) (mapm/accumulate-builds (lambda (hook) (hook manifest)) hooks)))) ...) --8<---------------cut here---------------end--------------->8--- At this point, I thought: “Well, I could just parameterize %current-system inside the (lambda (hook) ...), and all would be well, right? Well, it didn't seem to work and I was pretty confused by it. I tested a bit and noticed that actually, contrary to what was intended (there even is a comment in gexp-derivation about it), gexp-derivation looks up the system at monadic run time! It looks like this: --8<---------------cut here---------------start------------->8--- (mlet* %store-monad ( ;; The following binding forces '%current-system' and ;; '%current-target-system' to be looked up at >>= ;; time. (graft? (set-grafting graft?)) (system -> (or system (%current-system))) (target -> (if (eq? target 'current) (%current-target-system) target)) ...) ...) --8<---------------cut here---------------end--------------->8--- Well, the issue here is that such an mlet starts by translating the graft? binding into a >>= call, which ends up putting the rest of the translation into a function call that will *not* be called until the monadic value is run. That means that the system and target bindings afterwards are *not* looked up at call time but at monadic run time! And sure enough, moving the (graft? ...) binding after the (target -> ...) one does solve this! IMO, this is way too complicated to keep in mind at all times, and there are bugs lurking under the surface absolutely everywhere, waiting for a corner case to be uncovered. I'll send a new patch once I've fixed and tested this further. Best, -- Josselin Poiret [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 682 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#65225: [PATCH] environment: Build the profile for the requested system. 2023-08-12 10:33 ` Josselin Poiret via Bug reports for GNU Guix @ 2023-08-12 10:42 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-12 20:51 ` Ludovic Courtès 1 sibling, 0 replies; 7+ messages in thread From: Josselin Poiret via Bug reports for GNU Guix @ 2023-08-12 10:42 UTC (permalink / raw) To: Tobias Geerinckx-Rice, 65225 Cc: Tobias Geerinckx-Rice, Simon Tournier, Mathieu Othacehe, Ludovic Courtès, Christopher Baines, Ricardo Wurmus [-- Attachment #1: Type: text/plain, Size: 2582 bytes --] Hi everyone, Actually, let me add a big erratum, since it appears I misread the intention of gexp->derivation (and realized 2 minutes after writing the previous email). Josselin Poiret <dev@jpoiret.xyz> writes: > --8<---------------cut here---------------start------------->8--- > (mlet* %store-monad ( ;; The following binding forces '%current-system' and > ;; '%current-target-system' to be looked up at >>= > ;; time. > (graft? (set-grafting graft?)) > > (system -> (or system (%current-system))) > (target -> (if (eq? target 'current) > (%current-target-system) > target)) > ...) > ...) > --8<---------------cut here---------------end--------------->8--- > > Well, the issue here is that such an mlet starts by translating the > graft? binding into a >>= call, which ends up putting the rest of the > translation into a function call that will *not* be called until the > monadic value is run. That means that the system and target bindings > afterwards are *not* looked up at call time but at monadic run time! This is actually what the comment above hints at, I misunderstood its meaning. It seems that this piece of code used to be (before 2015) --8<---------------cut here---------------start------------->8--- (mlet* %store-monad ( ;; The following binding forces '%current-system' and ;; '%current-target-system' to be looked up at >>= ;; time. (unused (return #f) (system -> (or system (%current-system))) (target -> (if (eq? target 'current) (%current-target-system) target)) ...) ...) --8<---------------cut here---------------end--------------->8--- probably at a time when (current-system) didn't exist. In turn, this means that gexp->derivation intentionally delays getting the current system to monadic run time. Thus, we probably need to pass an optional #:system argument to the hooks that they can forward to gexp->derivation to fix this “properly” > IMO, this is way too complicated to keep in mind at all times, and there > are bugs lurking under the surface absolutely everywhere, waiting for a > corner case to be uncovered. My comment still stands. Best, -- Josselin Poiret [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 682 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#65225: [PATCH] environment: Build the profile for the requested system. 2023-08-12 10:33 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-12 10:42 ` Josselin Poiret via Bug reports for GNU Guix @ 2023-08-12 20:51 ` Ludovic Courtès 1 sibling, 0 replies; 7+ messages in thread From: Ludovic Courtès @ 2023-08-12 20:51 UTC (permalink / raw) To: Josselin Poiret Cc: Christopher Baines, Simon Tournier, Mathieu Othacehe, Tobias Geerinckx-Rice, 65225, Ricardo Wurmus Hi, Josselin Poiret <dev@jpoiret.xyz> skribis: > IMO, this is way too complicated to keep in mind at all times, and there > are bugs lurking under the surface absolutely everywhere, waiting for a > corner case to be uncovered. Great writeup! Yeah, that’s the sad situation of mixing parameters and monads; it’s a longstanding issue and as you write, there have been bugs. The solution to these has been primarily to avoid relying on default values and pass explicit values. The other trick is what ‘lower-gexp’ does, with the comment you quoted, or what ‘gexp->derivation’ does. The good news is that the monadic interface is kept internal and not really exposed to users, who should stick to file-like objects as much as possible. Back to profile hooks: it seems that passing #:system #f to ‘gexp->derivation’ in each of these hooks would solve the problem, no? Alternatively, each hook could take ‘system’ as a second argument. Willing to give it a shot? Thanks, Ludo’. ^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#65225: ‘guix shell --system=ALIEN’ builds for both systems 2023-08-11 11:10 bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-06 0:00 ` bug#65225: [PATCH] environment: Build the profile for the requested system Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-11 11:43 ` Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-11 15:58 ` Tobias Geerinckx-Rice via Bug reports for GNU Guix 1 sibling, 1 reply; 7+ messages in thread From: Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-11 11:43 UTC (permalink / raw) To: 65225 Oh, OK. Parts of scripts/shell.scm (and perhaps environment.scm) assume that $ guix build --system={x86_64,riscv64}-linux foo is equivalent to $ guix build --system=riscv64-linux foo It's not. This includes and causes things like commit 9c513303156b418567b9d2cde9f8df66190051ac. There's a lot of fast & loose assoc-reffing going on here that makes me nervous. Anyway, I'll get me to a PC and then fix the code that's injecting a spurious extra ‘(system . (%current-system))’ that's causing these double builds. Simple™. Kind regards, T G-R Sent from a Web browser. Excuse or enjoy my brevity. ^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#65225: ‘guix shell --system=ALIEN’ builds for both systems 2023-08-11 11:43 ` bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-11 15:58 ` Tobias Geerinckx-Rice via Bug reports for GNU Guix 0 siblings, 0 replies; 7+ messages in thread From: Tobias Geerinckx-Rice via Bug reports for GNU Guix @ 2023-08-11 15:58 UTC (permalink / raw) To: 65225 Oh OK. > the code that's injecting a spurious extra ‘(system . > (%current-system))’ that's causing these double builds is not actually the problem. Instead, the profile hooks are unconditionally built for the host's (%current-system). Forcing them to match ‘--system=’ does the trick. Simple™! Kind regards, T G-R Sent from a Web browser. Excuse or enjoy my brevity. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-08-12 20:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-11 11:10 bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-06 0:00 ` bug#65225: [PATCH] environment: Build the profile for the requested system Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-12 10:33 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-12 10:42 ` Josselin Poiret via Bug reports for GNU Guix 2023-08-12 20:51 ` Ludovic Courtès 2023-08-11 11:43 ` bug#65225: ‘guix shell --system=ALIEN’ builds for both systems Tobias Geerinckx-Rice via Bug reports for GNU Guix 2023-08-11 15:58 ` Tobias Geerinckx-Rice via Bug reports for GNU Guix
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).