Skip to content

[core] Catch errors in package init functions and when loading files #16952

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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

[core] Catch errors in package init functions and when loading files #16952

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af7981f
[core] Catch errors in package init functions and when loading files
fnussbaum Apr 11, 2025
e36fe6c
[core] Directly use `load` for loading lock file
fnussbaum Apr 11, 2025
e1bc953
[core] Remove superfluous newlines from error messages
fnussbaum Apr 11, 2025
dc8c133
Remove superfluous quotes from conditions
fnussbaum Apr 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions core/core-configuration-layer.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ cache folder.")

(defun configuration-layer/load-lock-file ()
"Load the .lock file"
(configuration-layer/load-file configuration-layer-lock-file))
(load configuration-layer-lock-file nil (not init-file-debug)))

(defun configuration-layer/initialize ()
"Initialize `package.el'."
Expand Down Expand Up @@ -1574,10 +1574,10 @@ RNAME is the name symbol of another existing layer."
(set-default var val))
('error
(configuration-layer//error
(concat "\nAn error occurred while setting layer "
(concat "An error occurred while setting layer "
"variable %s "
"(error: %s). Be sure to quote the value "
"if needed.\n") var err)))
"if needed.") var err)))
(configuration-layer//warning "Missing value for variable %s !"
var))))))

Expand Down Expand Up @@ -1691,7 +1691,7 @@ RNAME is the name symbol of another existing layer."
pkg-name)))
('error
(configuration-layer//error
(concat "\nAn error occurred while installing %s " "(error: %s)\n")
(concat "An error occurred while installing %s " "(error: %s)")
pkg-name
err)
(spacemacs//redisplay))))))
Expand Down Expand Up @@ -2006,8 +2006,8 @@ LAYER must not be the owner of PKG."
(funcall (intern (format "%S/pre-init-%S" layer pkg-name)))
('error
(configuration-layer//error
(concat "\nAn error occurred while pre-configuring %S "
"in layer %S (error: %s)\n")
(concat "An error occurred while pre-configuring %S "
"in layer %S (error: %s)")
pkg-name layer err))))))
(oref pkg :pre-layers))))

Expand All @@ -2018,7 +2018,13 @@ LAYER must not be the owner of PKG."
(owner (car (oref pkg :owners))))
;; init
(spacemacs-buffer/message (format "%S -> init (%S)..." pkg-name owner))
(funcall (intern (format "%S/init-%S" owner pkg-name)))))
(condition-case-unless-debug err
(funcall (intern (format "%S/init-%S" owner pkg-name)))
('error
(configuration-layer//error
(concat "An error occurred while configuring %S "
"in layer %S (error: %s)")
pkg-name owner err)))))

(defun configuration-layer//post-configure-package (pkg)
"Post-configure PKG object, i.e. call its post-init functions."
Expand All @@ -2035,8 +2041,8 @@ LAYER must not be the owner of PKG."
(funcall (intern (format "%S/post-init-%S" layer pkg-name)))
('error
(configuration-layer//error
(concat "\nAn error occurred while post-configuring %S "
"in layer %S (error: %s)\n")
(concat "An error occurred while post-configuring %S "
"in layer %S (error: %s)")
pkg-name layer err))))))
(oref pkg :post-layers))))

Expand Down Expand Up @@ -2800,7 +2806,12 @@ ARGS: format string arguments."

(defun configuration-layer/load-file (file &optional noerror)
"Load file silently except if in debug mode."
(load file noerror (not init-file-debug)))
(condition-case-unless-debug err
(load file noerror (not init-file-debug))
('error
(configuration-layer//error
"An error occurred while loading %S (error: %s)"
file err))))
Copy link
Collaborator

@bcc32 bcc32 Apr 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this change is a good idea. configuration-layer/load-file is used in several places that IMO should simply propagate any errors loading the file. For example in configuration-layer/rollback and configuration-layer/load-auto-layer-file (and maybe configuration-layer/make-layer).

It is also unfortunately a "public" function based on naming convention, so one more reason to maybe change the callsites rather than the function itself (although I consider this a weaker reason).

Copy link
Collaborator Author

@fnussbaum fnussbaum Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configuration-layer/load-file is used in several places that IMO should simply propagate any errors loading the file.

In configuration-layer/rollback we should definitely propagate the error: State from a previous invocation could be left in update-packages-alist, which would execute those previous rollbacks again.

In configuration-layer/load-auto-layer-file and configuration-layer/make-layer I don't immediately see the necessity to propagate the errors. Do you think something bad could happen if we continue in case of errors? In particular, could failing to load a single file be realistically worse than not loading any other file after an error? I guess it could be, but I'm not convinced yet.


(provide 'core-configuration-layer)

Expand Down