Skip to content

A minor problem in tools/install-packages.el #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
borderite opened this issue Apr 14, 2025 · 3 comments
Closed

A minor problem in tools/install-packages.el #469

borderite opened this issue Apr 14, 2025 · 3 comments

Comments

@borderite
Copy link
Contributor

It seems that the function exercism//install-required-packages in tools/install-packages.el assumes that if the variable package-archive-contents is not nil, the variable package-archives contains the gnu and melpa site information. This assumption does not hold in general.

For example, an emacs user has run (package-refresh-contents) without explicitly setting package-archives. In this case, package-archives contains the "gnu" and "nongnu" sites but not the "melpa" site, so that package-archive-contents is filled with the information downloaded from the "gnu" and "nongnu" sites. But two among the three required packages, mustache and string-inflection, are only avialble at the melpa site. So, running exercism//install-required-packages will result in an error message, not being able to install mustache or string-inflection, unless both of them have been already installed.

The problem can be fixed easily. Intead of checking if package-archive-contents is nil, we can directly check if the three required packages have been installed. The resulting code loos like:

(defun exercism//install-required-packages ()
  (require 'package)
  (package-initialize)
  (let ((required-packages '(mustache ht string-inflection)))
    (when (not (cl-every #'package-installed-p required-packages))
      (add-to-list
       'package-archives '("gnu" . "https://elpa.gnu.org/packages/")
       t)
      (add-to-list
       'package-archives '("melpa" . "https://melpa.org/packages/")
       t)
      (package-refresh-contents)
      (dolist (pkg required-packages)
	(package-install pkg)))))

Does this look OK? If so, shall I submit a PR?

@fapdash
Copy link
Contributor

fapdash commented Apr 14, 2025

Did you run into this issue?

Usually the code is called via emacs -batch, so without init file. In that case package-archive-contents should always be nil. But I guess I also tried to run this code from my normal Emacs session and added the check for package-archive-contents. I don't remember what my thinking was there. 🤷

Feel free to open a PR, just make sure to add (require 'cl-extra), otherwise the cl-every call might fail.
cl-extra requires both cl-lib and seq, so you might as well use seq-every-p and only (require 'seq). But feel free to keep cl-every. This code isn't running in CI, we don't have to fight for every ms, I'm just being pedantic. ;)

@borderite
Copy link
Contributor Author

@fapdash Yes, I actually got into the problem. Initially, I also thought that package-archive-contents should be nil if the code is run via emacs -batch. I even tried adding "-q" to the emacs commandline option. It seems that Emacs always getting the archived contents from ~/.emacs.d, at least, on my machine (Debian Linux 12).

I have added (require 'cl-extra) to the code. I will submit the PR shortly.

fapdash pushed a commit that referenced this issue Apr 27, 2025
- The function exercism//install-required-packages now perform
installation only if at least one of thte required applications has
not be installed.
@fapdash
Copy link
Contributor

fapdash commented Apr 27, 2025

Fixed via #470, closing

@fapdash fapdash closed this as completed Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants