Closed haji-ali closed 2 years ago
For me,
(featurep 'el-patch)
returns nil after init, unlessel-patch
was rebuilt bystraight.el
due to modifications to its source code. Remember,(straight-use-package 'el-patch)
doesn't actually loadel-patch
: it only adds the package toload-path
, so that it can be loaded if requested.
Ah yes, I didn't notice before, but el-patch
was actually being automatically loaded because I was using it. Please disregard this comment.
One minor comment:
If we disallow-list things, it's not possible to be achieve a state where you're safe against radian pulling in new unwanted features.
If we allow-list things, you can (setq radian-features '(my features))
to get that behavior, but still easily achieve the current behavior by just removing unwanted things as in (setq radian-features (delq 'my-feature radian-features))
.
Maybe even provide (defun radian-disable-feature (feature-sym)) (setq radian-features (delq feature-sym radian-features))
? Or let it take a list...
Hey @haji-ali, sorry for the delay, I have returned to civilization.
disallow-list vs allow-list
I think this might be a case where we can eat our cake and have it too! We could have a variable radian-disabled-packages
as present, but that list can start with :not
, in which case it changes from a denylist to an allowlist. I've seen this pattern at least once before in Emacs package configuration, though I can't remember where off the top of my head.
Maybe even provide
radian-disable-feature
Yeah, we could definitely do that!
@raxod502, I just forced pushed the suggestions. radian-disabled-packages
can now start with :not
as suggested.
I still need to figure out two things:
radian-use-package
, I need to correctly extract the recipe from the use-package
arguments.radian-org-enable-contrib
was nil, org-contrib
was not registered. Currently, I do register this package (as I do all disabled packages) which causes an error (hence all the build failures). I am unsure what is the correct thing to do here. I thought registering the package was necessary.Another point for consideration.
For some reason when compiling radian.el
, I get error of the form cannot load package-X
for disabled packages. I am not sure why disabled packages are being loaded during compilations but not during execution.
I apologize for the great delay here. I am going to take a look at this PR and see if I can't finish it up on my end.
Verified that (setq radian-disabled-packages '(magit magit-diff forge))
successfully appears to disable Magit configuration at runtime. However, I do see lots of byte-compilation errors. Will investigate.
Ah, so the issue here is basically that since the package is not loaded, Emacs cannot verify that any of the functions in the package configuration are defined, hence warnings get thrown.
The answer to why the packages are getting loaded is pretty simple---that's a default behavior of use-package
, it tries to load the package at byte-compile time for exactly this reason (to avoid byte-compilation warnings and possible wrong macroexpansion in some rare edge cases). Even though the use-package
form is nested inside an if
branch that isn't touched at runtime, Emacs does not know that at byte-compilation time (and with the current design, radian-disabled-packages
could actually be determined at runtime).
I'm not sure what would be the right thing to do here. Will think on it.
One option would be to load all packages during byte-compilation, ignoring radian-disabled-packages
. However, this has some undesirable side effects, including:
The fundamental issue here is that radian-disabled-packages
is customizable at runtime, but we need to know whether a package is disabled at compile time. The most appropriate solution seems to me to be to force radian-disabled-packages
to be customizable only statically at the beginning of init. This should be possible. I will investigate.
I came up with a workaround. We can sidestep the problem by observing that while we cannot byte-compile a package's configuration without having that package loaded, this does not make the configuration invalid. Therefore, if the package is not available at byte-compile time, we can simply refrain from byte-compiling the configuration, and leave it unchanged. I implemented a wrapper macro that does the necessary feature detection, and this appears to fully resolve the problem described above without introducing any new constraints on configuration.
However, disabling one package may still introduce byte-compilation warnings in other packages' configuration. For example, disabling Magit as described above leads to a warning here, which is completely legitimate and expected from a correctness standpoint, but not good from a user standpoint: https://github.com/raxod502/radian/blob/9f7891c52cac04e49b95c4ce93d3bc18bef9ab0b/emacs/radian.el#L4849-L4854
Some thought will be needed to figure out how to handle these cross-package configuration dependencies in general, or if there is a good way to do so.
This should be straightforward to address on a package-by-package basis. I have pushed a commit which corrects the issue with disabling Magit. I tested disabling Org and it appears to work fine. With that, I think this feature looks basically correct. @haji-ali, would you be able to see if it works in your setup and for all the desired packages?
@raxod502, thanks for these fixes and the nice approach to fixing the compilation!
I just had time to try out this feature, sorry for the delay. I noticed the following problems;
git-gutter
, outline
, autorevert
, org-clock
and dired
gitconfig-mode
and gitignore-mode
. These maybe should be replaced (perhaps in the main branch) with git-modes
.helpful
is used inside elisp-mode
, no-littering
is used inside auth-source
(I fixed this by setting the value of radian--auth-source-blacklist-file
before defining it, like no-littering
does with other variables) and delsel
is used in the advice radian--advice-keyboard-quit-minibuffer-first
(the function minibuffer-keyboard-quit
is defined in delsel
). I don't disable these packages so this does not affect me, but thought you might want to know.radian-protect-macros
results in a problem where lexical binding is not enabled for these forms. This appears with this pull-request even on an empty init.local.el
where I get the error Error running timer: (void-variable func)
in the advice for save-buffers-kill-emacs
when trying to exit Emacs.Alrighty, I think all those issues should now be fixed. Thoughts?
Ugh, they renamed functions again between Emacs versions---have to add a switch between abort-minibuffers
and whatever it used to be called. Will tackle that at some point, if you don't get to it first.
Great! Just tested and everything works as expected. A couple of thoughts/changes:
helpful
but enabling elisp-mode
might lead to a problem since helpful-*
functions won't be defined. I pushed a slight modification to handle this, please see if it is agreeable.t
(or lexical-binding
) as the second argument of eval
to enable lexical binding. I changed this in this pull request. There are other calls to eval
in radian.el
that could benefit from a similar treatment, but I am not sure where it's actually needed.LGTM! Thanks!
Implemented ideas from #475.
As an example, here's the setting that causes Radian not to load any packages (Except for the mandatory
straight
,use-package
,blackout
,bind-key
andel-patch
).PS: This is not relevant to this pull request. But I find it strange that
el-patch
is loaded even whenradian.el
is compiled. Perhaps loading the package should be excluded from the compiled file? But then it would be automatically pruned which might be undesirable.