Make install check aware of alternative package managers#1007
Conversation
rswgnu
left a comment
There was a problem hiding this comment.
You are on the right track. I would finish it off and add quelpa.
754e1c6 to
01440cb
Compare
rswgnu
left a comment
There was a problem hiding this comment.
Let's see together if we can reduce the number of these added support functions before merging this. We can go through it together when you have time.
What functions do you think makes sense to merge? Do you think the number of functions makes the code hard to understand? |
|
Just felt like a lot of functions when I first looked at it. I'll have to look at it more to figure out where we could merge in consultation with you. |
There are four new functions. But shall the number of functions matter? Yes, they are small but with a clear purpose. The ChangeLog is a bit hard to read but I tried to get all in there which might make it look more than it is!? Happy talk about it. |
69b4a85 to
27e61c3
Compare
|
@rswgnu I've rebased it so it is possible to merge it so it will be possible for you to add require of denote if you want. |
ce976bd to
b4f668b
Compare
|
I don't understand your reference to denote here. Could you explain or point me to some other message you are referring to. I do see that the latest merge build here failed on 28.2. Are you fixing that? |
|
@rswgnu Removing the nongnu archive insertion that we talked about yesterday did not work. Reason was that in one call path the whole package-archives was locally set to nil. I suppose for not tampering with users package-archives. I had missed that. So I inserted that back but slightly different. As we said yesterday we possibly can remove this juggling since with nongnu being the default we could just try to install. However I did not want to do that in this long and winding PR. Happy to work on that in a separate PR though. I also added unit tests for the new functions. Since the PR had many commits I squashed it and tried to make a better ChangeLog for the changes. PTAL. |
|
As we said yesterday we possibly can remove this juggling since with
nongnu being the default we could just try to install. However I did not
want to do that in this long and winAs we said yesterday we possibly can
remove this juggling since with nongnu being the default we could just try
to install. However I did not want to do that in this long and winding PR.ding
PR.
If you are just going to rely on default behavior, I would deal with that
now and then have your tests cover it, so all of these updates are tested
in one go. I have sent my other review comments. I'll try to wrap this up
with you today.
…On Thu, Jul 16, 2026 at 9:54 AM Mats Lidell ***@***.***> wrote:
*matsl* left a comment (rswgnu/hyperbole#1007)
<#1007 (comment)>
@rswgnu <https://github.com/rswgnu> Removing the nongnu archive insertion
that we talked about yesterday did not work. Reason was that in one call
path the whole package-archives was locally set to nil. I suppose for not
tampering with users package-archives. I had missed that. So I inserted
that back but slightly different. As we said yesterday we possibly can
remove this juggling since with nongnu being the default we could just try
to install. However I did not want to do that in this long and winding PR.
Happy to work on that in a separate PR though.
I also added unit tests for the new functions.
Since the PR had many commits I squashed it and tried to make a better
ChangeLog for the changes.
PTAL.
—
Reply to this email directly, view it on GitHub
<#1007?email_source=notifications&email_token=AE5WPD5KXIB3UCHUHKRIFRD5FDNARA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJZGI3TCNBSGEY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4992714211>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE5WPD2KX5WBGUPCIUKRWBD5FDNARAVCNFSNUABEKJSXA33TNF2G64TZHM3DIMZVGEYDQNR3JFZXG5LFHM2DQNRSGU2TKMJVG2QXMAQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Well, I don't think the tests need any change. It is two line change and all tests works when I tried it. I can push it so you can review. |
953f69c to
87790f1
Compare
I understood it as you said you were pausing adding the dynamic install check for denote until this PR was merged so you would not make this work harder. Rather to be able to use hybp:install-package directly when it was ready. Maybe I misunderstood. |
|
Don't know if we can trust this but this time the build worked for all versions. PTAL |
|
I approved so you can merge when you have time to test. |
Try to identify when user does not use the default package.el and then
fall back to notify users a manual install of a dynamic dependency is
neeed. Do also ask a user that uses the packet.el before doing the
default, customizable to not ask.
* test/hy-test-helpers.el (hy-test-mocked-feature): Macro for mocking
featurep.
* test/hypb-tests.el (hypb--users-package-manager):
(hypb--package-el-install):
(hypb--notify-manual-install-needed):
(hypb--ensure-dependency):
(hypb--require-package): Unit tests for the new functions.
* test/hy-test-dependencies.el (hypb:ask-to-install-package-flag): Set to
nil for not asking about installing dynamic dependencies during test,
i.e. markdown-mode.
* hsys-consult.el (hsys-consult-require-version):
(hsys-consult--org-roam-call-function):
* hui-treemacs.el (treemacs):
* hypb.el (hypb:activate-interaction-log-mode): Use hypb:require-package.
* hyrolo.el (hyrolo-any-file-type-problem-p): Use require to check if
markdown-mode is available. Ensure package-archives is set to nongnu
for markdown-mode install. Moved out from
hyrolo-install-markdown-mode.
(hyrolo-markdown-mode): Remove not needed require.
(hyrolo-install-markdown-mode): Use hypb:require-package.
* hypb.el (hypb:ask-to-install-package-flag): Give user control if package
shall be installed when using package.el.
(hypb:users-package-manager): Users package manager.
(hypb:package-el-install): Install package using package.el.
(hypb:notify-manual-install-needed): Notify user manual install is needed.
(hypb:ensure-dependency): Ensure package is available using
hypb:package-el-install or hypb:notify-manual-install-needed.
(hypb:require-package): Use hypb:ensure-dependency.
If user has removed nongnu and use the default package manager an install error will notify user that markdown mode is needed.
87790f1 to
b86dfe8
Compare
In CI we are seeing odd internal errors due to wrong-type-argument in package.el. This adds an advice to retry this case. It is applied as a band aid to unblock CI for now on this issue. * test/hy-test-dependencies.el (hypb:package-install-advice-for-retry): Advice for retrying package-install.
|
@rswgnu This was the least intrusive change I could come up with. Slightly sad things is that with this advice the error has never occurred. It is like Heisenberg's indeterminacy principle. If we try to catch the error it is gone. I know the PR is approved but want you view on this way to move forward since that was added after the approval. |
What
Make install check aware of alternative package managers and avoid doing
package-install for those. Adding a
defcustomso users can choose to get anautomated install if using the standard
package.el.Why
There are multiple package managers so we can't assume users use
package.el. If it is not used then using it to install a package would likely
be an error an could damage a users setup or at least not be what the user
expects.
Supporting different package managers for an automatic install is beyond
where we want to go.
Note
The idea is to use
hypb:ensure-dependencyin all places where we today topackage-install-p. That would beBefore implementing that could you, @rswgnu, take a look at what you think of
using this idea? If that makes sense I can complete this Draft.