Skip to content

Conversation

@fnussbaum
Copy link
Collaborator

(We already similarly catch errors in pre-init and post-init functions.)

Errors can happen, for example, when accidentally introducing a typo in some (private or public) layer file; or when adding the mu4e layer without having the mu4e package installed (or mu4e-autoloads.el is missing).

Such errors currently completely break the startup process, such that most keybindings don't work, which makes it more tedious to actually solve the problems.

(We already catch errors in pre-init and post-init functions.)

Errors can happen, for example, when accidentally introducing a typo in
some (private or public) layer file; or when adding the mu4e layer without
having the mu4e package installed (or mu4e-autoloads.el is missing).

Such errors currently completely break the startup process, such that most
keybindings don't work, which makes it more tedious to actually solve the
problems.
Because the lock file is loaded before `dotspacemacs/user-init` is called, and
due to syl20bnr#16871, I would not introduce a condition-case-unless-debug form here. It
would make it difficult to obtain a backtrace in case of an error (in
particular, setting `debug-on-error` in `dotspacemacs/user-init` would not work,
and `--debug-init` does not work, either).
They mess up the layout of the messages in the Spacemacs buffer.
('error
(configuration-layer//error
"An error occurred while loading %S (error: %s)"
file err))))
Copy link
Collaborator

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

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.

@fnussbaum fnussbaum marked this pull request as draft April 13, 2025 22:24
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

Successfully merging this pull request may close these issues.

2 participants