-
Notifications
You must be signed in to change notification settings - Fork 4.9k
[core] Rework dotspacemacs-sync-packages behavior for site packages #16928
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
[core] Rework dotspacemacs-sync-packages behavior for site packages #16928
Conversation
b5378af
to
234761d
Compare
234761d
to
61add73
Compare
core/core-configuration-layer.el
Outdated
|
||
;; Do not noisily fail to delete system packages. | ||
(setq orphans | ||
(cl-delete-if (lambda (pkg-name) | ||
(let ((pkg-desc (cadr (assq pkg-name package-alist)))) | ||
(configuration-layer//system-package-p pkg-desc))) | ||
orphans)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually still necessary, after having declared site packages as distant? The PACKAGES passed to configuration-layer/delete-orphan-packages
should never end up in the orphans list (the docstring seems misleading here). And IIUC the function should always get called with all distant packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I finally understood that this is about unused site packages (which the commit message indeed says). An unused site package that is properly installed will end up in the orphans list when interactively calling configuration-layer/delete-orphan-packages
. But then, if a user actually does this, perhaps they would like to be notified about such unused site packages?
With this change, the orphans-count
still includes unused site packages. I guess this could look like a bug, so perhaps we should change this, or print some kind of message?
Perhaps we could consider one of the following alternatives:
- Move the
system-package-p
check toconfiguration-layer//package-orphan-p
? - Print a single message about the skipped site packages.
- Keep noisily trying to delete site packages (only in interactive calls, not on startup).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case is for site packages that are installed by an administrator in a shared place, which the user should not be notified about because they cannot deleted by the user. I would at least like to be able to preserve this property, perhaps making it configurable. (3) seems appealing, but I'll need to think about it a little further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. If you prefer, we can merge this first and then possibly improve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me so long to get back to this, I have been very busy the last few weeks. I have refactored this a bit and also made the change proposed in (3).
Even though packages with ":location site" cannot be installed from ELPA, they still need to be installed and present in package-alist in order to be configured correctly. This change fixes the following two bugs: 1. Used site packages that were present would be removed at startup, if `dotspacemacs-install-packages' was set to `used-only'. Of course, since the packages are typically installed at the system level, this would then noisily fail in `configuration-layer//package-delete'. After this commit, Spacemacs does not even consider these packages for deletion. 2. Used site packages that were missing failed to load in use-package but did not warn the user that the package failed to install in the first place. After this commit, Spacemacs more precisely informs the user that the package installation step went wrong, not load-path configuration.
61add73
to
bfa64b8
Compare
This makes a subsequent change easier, since determining whether a package is a system package requires the descriptor. It also lets us simplify configuration-layer//package-delete, which no longer needs to handle the case where a package is attempted to be deleted but does not exist.
This is noisy on startup and pretty pointless to warn users about. configuration-layer//package-delete will already skip such packages, but configuration-layer/delete-orphan-packages needs to be modified in order to avoid messaging about them still. When invoking the command interactively, continue to include system packages so that users who are interested can still find out about them (although Spacemacs will not attempt to actually delete them).
bfa64b8
to
9daf7bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me!
Lets merge and improve as we go along.
Thanks for the hard work :)
Fix #16898.
This PR consists of two commits that fix two separate bugs with site
packages. Please see the individual commit messages for details.