ryanprior / emacs-guix-packaging

Tools for writing and maintaining Guix packages
22 stars 3 forks source link

Accept multiple candidates in guix-packaging-insert-input #5

Closed Ambrevar closed 3 years ago

Ambrevar commented 3 years ago

Using completing-read-multiple should do the trick:

(defun guix-packaging-insert-input (package-strings)
  "Insert the corresponding Guix package input for PACKAGE-STRING.
eg. for ruby@2.7.2 insert (\"ruby@2.7.2\" ,ruby-2.7)."
  (interactive
   (let ((package-strings
          (completing-read-multiple "Search packages: "
                                    (cl-map 'list (-rpartial #'plist-get :name)
                                            (or guix-packaging--all-guix-packages
                                                (guix-packaging-refresh-packages))))))
     (list package-strings)))
  (dolist (package-string package-strings)
    (insert (guix-packaging--format-input package-string))
    (newline-and-indent)))
ryanprior commented 3 years ago

I tried this out, but I've got two issues with it: 1) When I do a normal completing-read, candidates show up as I type without me having to hit Tab, which I like. Using completing-read-multiple kicks me into a different UI where I have to hit Tab before I can see candidates. 2) I tried selecting 4 packages, and the performance was not great. It took a few seconds to insert them all as inputs. Maybe I can improve that performance somewhat: run just one guix command to get all the package locations, group them by file, process each group without redundantly reloading files. But at what cost in terms of complexity?

So, some questions to help me understand the request:

ryanprior commented 3 years ago

I've improved the performance somewhat. Here's what the breakdown looks like for inserting six inputs, five times:

function                                invocations   total time    avg time
guix-packaging-insert-input             5             3.5668748449  0.713374969
guix-packaging--format-inputs           5             3.553547574   0.7107095148
guix-packaging--guile-symbols           5             3.55345453    0.710690906
guix-packaging--invoke-guix             5             2.3401399919  0.4680279983
guix-packaging--goto-line               30            0.0183866150  0.0006128871

That seems pretty good, and came at only moderate additional complexity cost. I consolidated the processing so that only one call to Guix is required, but didn't batch the location of Guile symbols by file. Still, we can see that the single Guix call takes over half the time for the whole thing, so I don't expect that further I/O batching would improve performance much. And, at a sub-second timing for 6 inputs, I'm inclined to think that the performance is enough improved to defeat my no. 2 gripe above.

Gripe no. 1 remains, but it may be due to a shortcoming of Ivy if it just doesn't provide as rich an interface for completing-read-multiple.

Ambrevar commented 3 years ago

Ryan Prior notifications@github.com writes:

1) When I do a normal completing-read, candidates show up as I type without me having to hit Tab, which I like. Using completing-read-multiple kicks me into a different UI where I have to hit tab before I can see candidates.

I have the same issue with Helm and I'd be happy to know a fix :)

  • How much time will this save you to be able to multi-select packages here?

Lot's of keypresses, so probably 1-2 seconds per input, or 10-20 seconds for 10 inputs.

  • How many packages do you picture multi-selecting, ballpark - 2? 20?

Let's target 20, it sounds reasonable.

  • How slow does it have to be before you care about the performance?

If it can insert the 20 inputs in < 2s then I'm happy!

Ambrevar commented 3 years ago

Ryan Prior notifications@github.com writes:

Still, we can see that the single Guix call takes over half the time for the whole thing, so I don't expect that further I/O batching would improve performance much. And, at a sub-second timing for 6 inputs, I'm inclined to think that the performance is enough improved to defeat my no. 2 gripe above.

You can still improve the results if you use guix repl. On my machine, guix repl is 10 times (!) faster than guix <whatever>.

Yesterday some people helped me figure out a way to send multiple commands to a single guix repl a single time from Common Lisp:

(let ((pid (uiop:launch-program '("guix" "repl" "-t" "machine")
                                :input :stream :output :stream)))
  (format (uiop:process-info-input pid) "18~%")
  (format (uiop:process-info-input pid) "19~%")
  (close (uiop:process-info-input pid))
  ;; Skip REPL header:
  (read-line (uiop:process-info-output pid) nil :eof)
  (list
   (read-line (uiop:process-info-output pid) nil :eof)
   (read-line (uiop:process-info-output pid) nil :eof)))

; => ("(values (value 18)) (values (value 19))")

I think the same can be achieved in Emacs with make-process and make-pipe-process.

If you can't figure it out, then you can still fall back to guix repl /dev/stdin and get the result from the standard output.

In Common Lisp:

(uiop:run-program '("guix" "repl" "-q" "--type=machine" "/dev/stdin")
                        :input input-buffer
                        :output '(:string :stripped t))

Cheers!

ryanprior commented 3 years ago

I'm not really interested in using Guix internal APIs. The other project which does that, guix.el, is broken and has been for some time because guix repl is a moving target that's hard to track.

I explained on the guix-devel mailing list why I don't like guix.el's approach of using internal APIs via the repl, which to my surprise has prompted a flurry of activity to get it back in working order and establish a set of stable APIs for it to use. So I expect to take another trial period once that work has landed and reevaluate that approach.

In the near term I expect to remain committed to running Guix shell commands.

Ambrevar commented 3 years ago

Ryan Prior notifications@github.com writes:

I'm not really interested in using Guix internal APIs. The other project which does that, guix.el, is broken and has been for some time because guix repl is a moving target that's hard to track.

I don't believe this is true. guix repl and the API is stable and has been so for a while.

guix.el is broken for other reasons, such as interaction with Geiser, etc. Besides, guix.el does not use guix repl.

I explained on the guix-devel mailing list why I don't like guix.el's approach of using internal APIs via the repl, which to my surprise has prompted a flurry of activity to get it back in working order and establish a set of stable APIs for it to use.

Not exactly: the API is already stable, the question was about exporting some of the internal symbols that guix.el uses. Note that while implementing the Guix interface for Nyxt I didn't have to use a single unexported symbol.

You can safely rely on all exported symbols in Guix.