diff mbox series

[bug#68163] gnu: Prevent stale cache use when `%package-module-path' is parameterized.

Message ID b2aaf81a7ae81d47ff5091cb4012764124c6f61c.1704002781.git.antlers@illucid.net
State New
Headers show
Series [bug#68163] gnu: Prevent stale cache use when `%package-module-path' is parameterized. | expand

Commit Message

antlers Dec. 31, 2023, 6:06 a.m. UTC
* gnu/packages.scm(find-package-by-name/direct): Convert `delay'-ed cache
construction in closure into a form memoized over `%package-module-path'.

Change-Id: I6e4b4b3fa58082b79aacf307468aec43ec60bf22
---
This enables `specification->package' to be parameterized to resolve packages
from within the module-under-compilation.

I use this to parse my init.el[0] into packages in my guix-home profile, all
in one repo.

 gnu/packages.scm | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)


base-commit: 2a242e86379ebddbdddf2927f26e5e27a98fc605

Comments

Ludovic Courtès Jan. 8, 2024, 5:06 p.m. UTC | #1
Hi,

antlers <antlers@illucid.net> skribis:

> * gnu/packages.scm(find-package-by-name/direct): Convert `delay'-ed cache
> construction in closure into a form memoized over `%package-module-path'.
>
> Change-Id: I6e4b4b3fa58082b79aacf307468aec43ec60bf22
> ---
> This enables `specification->package' to be parameterized to resolve packages
> from within the module-under-compilation.
>
> I use this to parse my init.el[0] into packages in my guix-home profile, all
> in one repo.

Looks like you forgot reference [0].

(I’m not sure I understand the use case but seeing your code will surely
help!)

Thanks,
Ludo’.
antlers Jan. 8, 2024, 11:04 p.m. UTC | #2
So I did! It's probably better to give a clear, in-line example anyways.

I take it you know *what* I'm doing, just not *why*; but I'm going to
include a full example anyway for the benefit of future readers.

Note that this is a compile-time error, so profiles containing such
code fail to build entirely but produce cached object files which may
continue to function correctly after eg. a roll-back. I was briefly
bit by one after pulling an upstream profile to confirm the behavior
shown :p

``` scheme
#!/usr/bin/env -S guix repl -L ./modules
!#

(define-module (example)
  #:use-module (gnu packages)
  #:use-module (gnu packages base)
  #:use-module (guix packages))

(define-public my-hello
  (package
    (inherit hello)
    (name "my-hello")))

;; Suppose we're an average user and keep our configuration in a
;; channel in our home-dir:
(format (current-output-port) "~s~%"
  (module-filename (current-module)))
;; => "/home/user/src/example/./example.scm"

;; Whether or not it's best practice, we may be inclined to use
;; `specification->package' to populate our `operating-system',
;; `home-environment', and even (*gasp*) `service' records.
(format (current-output-port) "~s~%"
  (specification->package "hello"))
;; => #<package hello@2.12.1 gnu/packages/base.scm:92 7f958d8c09a0>

;; But this procedure fails when we attempt to reference a
;; package-variant defined within our configuration channel (even
;; when it's in another module, eg. `(example packages base)'):
(format (current-output-port) "~s~%"
  (specification->package "my-hello"))
;; => guix repl: error: my-hello: unknown package

;; (Note that this is a non-continuable exception, and would commented out
;; to execute the remaining stanza of this script.)

;; A clever user might parameterize `%package-search-path' to get
;; around this limitation:
(parameterize ((%package-module-path
                 (cons `(,(dirname (module-filename (current-module))) . "")
                       (%package-module-path))))
  (format (current-output-port) "~s~%"
    (specification->package "my-hello")))
;; => guix repl: error: my-hello: unknown package
;; But only with the patch do we enable this functionality:
;; => #<package my-hello@2.12.1 /home/user/src/example/./example.scm:10 7fc6a6678a50>
```

Regarding my specific use-case, I have a channel featuring the following
at least the following four snippets.

antlers/home/files/emacs/init.el[1]:
``` elisp
(use-package all-the-icons
  :guix (emacs-all-the-icons)
  :defer t)

(use-package magit
  :guix emacs-magit git
  :custom (magit-diff-refine-hunk t))

(use-package embark
  :guix (emacs-embark ; This refers to my fork with a page-able which-key pop-up on `embark-collect'
          --with-git-url=emacs-embark=file:///home/antlers/[...]
          --with-branch=emacs-embark=fix/issue-647)
  :custom (prefix-help-command 'embark-prefix-help-command))

(use-package devil-mode
  :guix emacs-devil-mode
  :custom (devil-key "'"))
```

antlers/home/extract-emacs-packages.scm[2]:
``` scheme
(define extact-emacs-packages (file-path)
  [...]) ; this is not polished code, you don't wanna see it: just imagine
;; => (list #<package [...]> ...)
```

antlers/packages/emacs-xyz:
``` elisp
(define-public emacs-devil-mode
  (package
    (name "emacs-devil-mode")
    (version "0.6.0")
    (source
     (origin
       (method git-fetch)
       (uri (git-reference
             (url "https://github.com/susam/devil")
             (commit version)))
       (sha256
        (base32 "1pr9yf6f37sz5qy1snn8ag5bvg6lza7q635jh8jhaqqfp37jvv1y"))
       (file-name (git-file-name name version))))
    (build-system emacs-build-system)
    (home-page "https://susam.github.io/devil")
    (synopsis "Modifier-free editing in Emacs")
    (description
     "Devil mode trades your comma key in exchange for a modifier-free
editing experience in Emacs.")
    (license license:expat)))
```

antlers/home.scm:
``` scheme
(simple-service 'emacs-packages home-profile-service-type
  ;; Add `(antlers packages)' to `specification->package' PATH
  (parameterize ((%package-module-path
                   (cons `(,(dirname (dirname (module-filename (current-module))))
                           . "antlers/packages")
                         (%package-module-path))))
    (append (extract-emacs-packages "./home/files/emacs/init.el")
            (extract-emacs-packages "./home/files/emacs/early-init.el"))))
```

If there's a better way to do this (barring the obvious of upstreaming
assorted bits) let me know, I'm sure there are many ways, but this
one's mine. I don't like to keep the manifest seperate from my configuration,
but also don't want to keep them in an org file and have to invoke
emacs to build my channel.

I don't know if a.) there's any cause to be concerned about the
performance of this tweak, or b.) that you care to support the
behavior at all, but I hope I've made my own use-case clear and the
patch is there if you see fit c:

Thx for your time!

1: https://gist.github.com/AutumnalAntlers/efa81fbb3ecc2c4fdd97785731f4348c
2: https://gist.github.com/AutumnalAntlers/b3090d73b97779f977105b905be14453
antlers Jan. 8, 2024, 11:56 p.m. UTC | #3
Some final thoughts I had I as I was moving-on:

1.) Mailing list guidelines prob say not to link to Gists, I made sure to include all relevant content in-line and wouldn't have attached them on their own anyways. They're there for the adventurous.

2) No, I don't want to put my packages in a separate channel-- I believe that the average Guix user has one channel for both their configuration and custom packages / services (I don't mean this in a demanding way, clearly happy to scratch my own itches, just anticipating the suggestion).

3.) Yes, my `extract-emacs-packages' parser really does support command-line transformation options and comments in use-package `:guix` blocks-- that snippet is verbatim.

4.) cc davie, he might get a kick outta that snippet

5.) I started preparing the reduced example as a `guix repl' session until I realized it has to be a real file for `module-filename' and `fold-packages' to work-- it could expose a module search-path, but I don't *think* there's a real use-case for that :p
Simon Tournier Jan. 11, 2024, 6:24 p.m. UTC | #4
Hi,

(For what it is worth, I have also read the rest of the thread. :-))

On Sun, 31 Dec 2023 at 06:06, antlers via Guix-patches via <guix-patches@gnu.org> wrote:

> -(define find-packages-by-name/direct              ;bypass the cache
> -  (let ((packages (delay
> -                    (fold-packages (lambda (p r)
> -                                     (vhash-cons (package-name p) p r))
> -                                   vlist-null)))
> +(define find-packages-by-name/direct
> +  ;; Bypass pre-built cache, but still memoize over `(%package-module-path)'
> +  (let ((packages (lambda ()
> +                    ((mlambda (_%package-module-path)
> +                       (fold-packages (lambda (p r)
> +                                        (vhash-cons (package-name p) p r))
> +                                      vlist-null))
> +                     (%package-module-path))))

I am not sure by this change.

 1. Instead, I would push to ’extract-emacs-packages’ the bits.
 2. I miss what makes your use case fails.

First, about #1.

Somehow, ’fold-packages’ accepts a list of modules as argument and it
would be the way you should go: in addition to the argument ’file-path’,
you should also pass to ’extract-emacs-packages’ another argument
capturing this locally extended %package-module-path.

Concretely, considering your example [1]: extract the list of packages
based on Emacs configuration (use-package) to then pass for “building
your home” using Guix.  From my understanding, the core reads:

--8<---------------cut here---------------start------------->8---
antlers/home/extract-emacs-packages.scm[2]:
``` scheme
(define extact-emacs-packages (file-path)
  [...]) ; this is not polished code, you don't wanna see it: just imagine
;; => (list #<package [...]> ...)
```

antlers/home.scm:
``` scheme
(simple-service 'emacs-packages home-profile-service-type
  ;; Add `(antlers packages)' to `specification->package' PATH
  (parameterize ((%package-module-path
                   (cons `(,(dirname (dirname (module-filename (current-module))))
                           . "antlers/packages")
                         (%package-module-path))))
    (append (extract-emacs-packages "./home/files/emacs/init.el")
            (extract-emacs-packages "./home/files/emacs/early-init.el"))))
```
--8<---------------cut here---------------end--------------->8---

I guess, the procedure ’extract-emacs-packages’ extracts the package
name represented as string from ’use-package’ keyword ’:guix’ and then
passes it to ’specification->package’ in order to build a list of
’<package>’ records.  Right?

If yes, I suggest to tweak that part instead of
’specification->package’.  When doing this conversion, from string to
<package>, it seems doable to:

 a) Pass a list of all modules to ’fold-packages’, i.e., get all the
 packages, included your custom ones;
 
 b) Filter based on package name (and/or other information tracked under
 use-package keyword :guix).

Somehow, do not rely on ’specification->package’ in
’extract-emacs-packages’.


Last, about #2.

What I do not understand is why is required: « This enables
`specification->package' to be parameterized to resolve packages from
within the module-under-compilation. »

When you run “guix pull”, it builds the Guix channel and all the other
custom channels, therefore all the packages should be visible from
’specification->package’.  Aren’t they?

Then, when running “guix home …”, I miss why it fails.

Other said,

 i)  Could you provide some details about the tree of your channel?

 ii) Could you explain when it fails exactly and running which Guix
     command-line?


Cheers,
simon

1: [bug#68163] [PATCH] gnu: Prevent stale cache use when `%package-module-path' is parameterized.
antlers via Guix-patches via <guix-patches@gnu.org>
Mon, 08 Jan 2024 23:04:41 +0000
id:21937d23-9a81-4090-979d-584f40880a51@app.fastmail.com
https://issues.guix.gnu.org/68163
https://issues.guix.gnu.org/msgid/21937d23-9a81-4090-979d-584f40880a51@app.fastmail.com
https://yhetil.org/guix/21937d23-9a81-4090-979d-584f40880a51@app.fastmail.com
antlers Jan. 13, 2024, 12:23 a.m. UTC | #5
> ’fold-packages’ accepts a list of modules as argument and it
> would be the way you should go

Thanks for the suggestion! I was so focused on making `specification->package'
cooperate, I failed to consider calling `fold-packages' directly; `#:modules'
is exactly the argument I was looking for. I've given it a spin and, while I
have some notes, it works great.

I still think it's worth considering whether the behavior of
`find-packages-by-name/direct' and it's closure in regards to the
parameterization of `%package-module-path' is intentional. A user's intuition
ought to be that dynamically binding a parameter will have the desired and
expected effect, down to the bottom of the call-stack, and
`find-packages-by-name/direct' is arguably presented as the implementation
"under" the caching layer. I'm not sure when it's internal cache is
instantiated, and find this behavior surprising.

Returning to `fold-packages' there are two differences of note, each stemming
from how `:guix' approximates guix's standard command-line format by
appropriating the existing modules.

1.) 

> I guess [...], right?

> If yes, [...] filter based on package name (and/or other information tracked
> under use-package keyword :guix).

Right! And, almost! Package specifications do not always match the
`package-name' field of the package they resolve to. Consider `package+output'
specifications like the venerable `gcc:lib' (RIP), and package aliases like
`gnupg-1', `linux-libre-lts', or `texinfo-7'. This is solve-able (maybe with
`package-name->name+version'), but it's neither as simple as filtering on
`package-name' nor as elegant as re-using the existing modules-- should they
behave appropriately under parameterization.

2)

I can avoid using `specification->package' directly, but would also need to
special-case `options->transformation' when the second argument to
`--with-graft` is such a specification because, unlike the other
transformations, it resolves that via `package->specification' before returning
a procedure (uhm, were I in need of such a transformation). Another solvable
problem, but also another example of an entry-point that could just
`do-what-I-mean' under parameterization-- and there are likely more.

Why rebuild two spokes when, with a small and deliberate tweak, the old wheel
could cooperate with an existing mechanism for exactly this sort of re-tooling?

---

I feel like that's a great place to leave off, but I appreciate your questions
and will take a bit more time answer the rest. 

>  i)  Could you provide some details about the tree of your channel?

Just that it's some custom packages, a home-env with local-files, and some
dubious programming, all in one channel. If the packages were in a separate
channel, or the home-env was kept outside of the repo and provided on the
command-line, everything would work just fine. The rest should be clear from my
elaborations.

> When you run “guix pull”, it builds the Guix channel and all the other
> custom channels, therefore all the packages should be visible from
> ’specification->package’.  Aren’t they?

>  ii) Could you explain when it fails exactly and running which Guix
>      command-line?

I have a typo in my original message:

> this is a compile-time error, so _profiles_ containing such code fail to build

It's *channels*, not profiles (and with the notable exception of guix itself),
that fail to compile when attempting to resolve specifications for packages
they define. We fail before Guix builds the package cache. It might help stage
this in a gexp, but `home-profile-service-type' takes a list (not a gexp) and
I'm not sure (yet) how to write the service I need or how that would effect
whether / when it fails. If I knew the package cache better I might know when /
what's missing it (aside from myself) and `force'-ing the cache in the closure
before I get there.

Oh my, time does fly!
Thanks again for the questions, and the help. I really could roll with
`fold-packages', but I'd rather keep the patch than build any more spokes. I
think the behavior is cleaner and I hope I've made a good case for that,
regardless of whether I'd recommend use-cases indulging in dynamic binding--
the parameter already exists and should treated as a volatile value.
antlers Jan. 13, 2024, 4:41 a.m. UTC | #6
> ii) Could you explain when it fails exactly and running which Guix
>     command-line?
 
Oh, I didn't answer this literally! Apologies for the extra mail, but I do have
an important clarification. I refer several times to "compile time" and
"compiling the channel"; what I am referring to is the lowering of it's
derivation, which happens before the package cache or profile is built when
running `guix pull` or `guix time-machine -C [...]`, not specifically the      
compilation of the SCM files comprising the channel (though my note about
having had to remove compilation artifacts from a successful build to surface
the issue after a rollback remains, and contributes to my confusion of terms).
antlers Jan. 17, 2024, 8:32 a.m. UTC | #7
Updates:

> I don't know if a.) there's any cause to be concerned about the
> performance of this tweak, or b.) that you care to support the
> behavior at all, but I hope I've made my own use-case clear and the
> patch is there if you see fit c:

a.) I glued hyperfine to a pair of guix repl endpoints, scaled up until I hit segfaults, no difference
b.) Maybe `specification->package' just has some particular notion of what's "current", like "current-channels" does, in which case we can rule out complacence with the parameter as a non-feature-- but I certainly isn't the same notion.
c.) I had the bright idea to set GUIX_PACKAGE_PATH at build time, first on the command-line and then in the channel modules, but it didn't work out-- felt silly for a minute there. Hey, how do third party channels usually resolve `search-path'? Isn't that the same thing, packages and patches in the same repo? Or are they just expected to use `local-file' and relative paths? I'll look into this another time.

If B or C had come to fruition I'd have closed the issue with 'em, and something like B is up for interpretation-- I don't mind either way (happy to carry the patch on my own). But if I enjoy these instances of syntax-sugar in my own channels (which I do :p), and if the Guix repo utilizes + is cleaner for them as well, then I figure it'd be a shame (barring a solution like C) to interpret the implementation as one which excludes other channels.
diff mbox series

Patch

diff --git a/gnu/packages.scm b/gnu/packages.scm
index 80c22d1d7f..b0cc2f7427 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -281,18 +281,21 @@  (define load-package-cache
                #f
                (apply throw args))))))))
 
-(define find-packages-by-name/direct              ;bypass the cache
-  (let ((packages (delay
-                    (fold-packages (lambda (p r)
-                                     (vhash-cons (package-name p) p r))
-                                   vlist-null)))
+(define find-packages-by-name/direct
+  ;; Bypass pre-built cache, but still memoize over `(%package-module-path)'
+  (let ((packages (lambda ()
+                    ((mlambda (_%package-module-path)
+                       (fold-packages (lambda (p r)
+                                        (vhash-cons (package-name p) p r))
+                                      vlist-null))
+                     (%package-module-path))))
         (version>? (lambda (p1 p2)
                      (version>? (package-version p1) (package-version p2)))))
     (lambda* (name #:optional version)
       "Return the list of packages with the given NAME.  If VERSION is not #f,
 then only return packages whose version is prefixed by VERSION, sorted in
 decreasing version order."
-      (let ((matching (sort (vhash-fold* cons '() name (force packages))
+      (let ((matching (sort (vhash-fold* cons '() name (packages))
                             version>?)))
         (if version
             (filter (lambda (package)