Skip to content
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

Can't get combobulate to work with load-path #119

Open
rdeusser opened this issue Oct 7, 2024 · 22 comments
Open

Can't get combobulate to work with load-path #119

rdeusser opened this issue Oct 7, 2024 · 22 comments

Comments

@rdeusser
Copy link

rdeusser commented Oct 7, 2024

I've got this in my Emacs config:

(use-package combobulate
  :custom
  ;; You can customize Combobulate's key prefix here.
  ;; Note that you may have to restart Emacs for this to take effect!
  (combobulate-key-prefix "C-c o")
  :hook ((prog-mode . combobulate-mode))
  ;; Amend this to the directory where you keep Combobulate's source
  ;; code.
  :load-path (lambda () (expand-file-name "combobulate.el" (straight--repos-dir "combobulate"))))

After restarting Emacs when I press C-c o it says it's undefined. If I run combobulate-mode through M-x nothing happens. Even with toggle-debug-on-error turned on nothing happens. There's nothing in *Messages* either. The only way I can get it to work is by doing this:

(use-package combobulate
  :straight (combobulate :type git :host github :repo "mickeynp/combobulate")
  :custom
  ;; You can customize Combobulate's key prefix here.
  ;; Note that you may have to restart Emacs for this to take effect!
  (combobulate-key-prefix "C-c o")
  :hook ((prog-mode . combobulate-mode))
  :config
  (load (expand-file-name "combobulate.el" (straight--repos-dir "combobulate"))))

but then I get a recursive load error in *Message*.

Anybody know why this is?

@matasar
Copy link

matasar commented Oct 7, 2024

I also have this problem. My experience here is that combobulate stopped working as of the more recent versions, and I can't work out how to make it work again.

@mickeynp
Copy link
Owner

mickeynp commented Oct 9, 2024

Clearly this third-party package manager does something different to the builtin use-package method of loading combobulate. Please use the provided example instead until I have time to look at a fix.

@gvoysey
Copy link

gvoysey commented Oct 15, 2024

This is not amazingly helpful, but the last working version i have pinned with straight is ee82c568ad639605518f62f82fae4bcc0dfdbb81, which is a lot to bisect.
(edit: it's now 573b74e, see below)

I can concur that i'm seeing this behavior too; C-c o o just doesn't exist as far as my emacs is concerned with:

  • GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.41, cairo version 1.18.0) of 2024-04-25
  • combobulate at a6aeea6 (most recent as of this writing)
  • straight.el at b3760f5829dba37e855add7323304561eb57a3d4 (fairly old; october 2023)

@gvoysey
Copy link

gvoysey commented Oct 15, 2024

i bisected it, though!

c66d23d is the first bad commit; 573b74e works.

status: waiting for both good and bad commits
~/.emacs.d/straight/repos/combobulate master|bisect ❯                                                                                                                                                                14:06:42
~/.emacs.d/straight/repos/combobulate master|bisect ❯ git checkout ee82c568ad639605518f62f82fae4bcc0dfdbb81                                                                                                          14:06:47
Note: switching to 'ee82c568ad639605518f62f82fae4bcc0dfdbb81'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at ee82c56 Merge pull request #85 from DamienCassou/readme-fixes
~/.emacs.d/straight/repos/combobulate @ee82c568|bisect ❯ git bisect good                                                                                                                                             14:07:09
status: waiting for bad commit, 1 good commit known
~/.emacs.d/straight/repos/combobulate @ee82c568|bisect ❯ git checkout master                                                                                                                                         14:07:12
Previous HEAD position was ee82c56 Merge pull request #85 from DamienCassou/readme-fixes
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
~/.emacs.d/straight/repos/combobulate master|bisect ❯ git bisect bad                                                                                                                                                 14:07:18
Bisecting: 43 revisions left to test after this (roughly 6 steps)
[1f3025e24b783334874e45406a7503b99e1d8c36] Try to ensure the right node order is used for enveloping
~/.emacs.d/straight/repos/combobulate @1f3025e2|bisect ❯ git bisect bad                         14:07:21
Bisecting: 21 revisions left to test after this (roughly 5 steps)
[1dde92d82e1de7657a172fefedfbbe506fee6b4a] Fix test deltas
~/.emacs.d/straight/repos/combobulate @1dde92d8|bisect ❯ git bisect bad                         14:08:48
Bisecting: 10 revisions left to test after this (roughly 3 steps)
[67e4634ba24d735859757057e25ca3e09edff18b] Actually use `rx' in combobulate-procedure-expand-rules
~/.emacs.d/straight/repos/combobulate @67e4634b|bisect ❯ git bisect good                        14:10:29
Bisecting: 5 revisions left to test after this (roughly 3 steps)
[889a464d05ae9dffbaa3a5b2654a426c76459ea6] Basic definition and procedure application now works
~/.emacs.d/straight/repos/combobulate @889a464d|bisect ❯ git bisect bad                         14:10:59
Bisecting: 2 revisions left to test after this (roughly 1 step)
[c66d23d6d3608f662b168b28b0539c9a74c34659] WIP define language
~/.emacs.d/straight/repos/combobulate @c66d23d6|bisect ❯ git bisect bad                         14:11:43
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[573b74e7359a254984036355678e54d236e11c4b] More improvements to Go sibling navigation
~/.emacs.d/straight/repos/combobulate @573b74e7|bisect ❯ git bisect good                        14:12:18
c66d23d6d3608f662b168b28b0539c9a74c34659 is the first bad commit
commit c66d23d6d3608f662b168b28b0539c9a74c34659
Author: Mickey Petersen <mickey@fyeah.org>
Date:   Fri Jan 26 05:40:40 2024 +0000

    WIP define language

 combobulate-settings.el | 23 -----------------------
 combobulate.el          | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 24 deletions(-)```

@matasar
Copy link

matasar commented Oct 16, 2024

Thanks for bisecting! I've pinned to that version and I can use it again. I also asked about this in the doom discord, maybe someone will have some ideas.

@gvoysey
Copy link

gvoysey commented Oct 16, 2024

doom's a whole other ball of wax -- luckily, you can eliminate it from contributing here @mickeynp because I don't use it (but do see the same failure to recognize C-c o o behavior)

Possibly a useful bit of information: i can make the transient display on the latest commit -- by first manually launching M-x combobulate-query-builder and then inside the query builder C-c o o is recognized and launches the main transient.

@matasar
Copy link

matasar commented Oct 17, 2024

Following up to confirm I'm happily combobulating again after pinning the commit @gvoysey bisected to.

@gvoysey
Copy link

gvoysey commented Oct 17, 2024

i had not realised quite how deeply it's gotten into my python workflow until it went on walkabout! so hats off to @mickeynp i think.

@mickeynp
Copy link
Owner

Everything should work fine with emacs -q and the instructions in the readme, though?

@mickeynp
Copy link
Owner

I had a go at fixing this. Straight does something funny with autoloading that normal emacs does not do. I tried figuring out why, but I found its "documentation" inscrutable.

The branch bugfix/fix-straight contains a potential fix.

Here is the code I used to test:

(straight-use-package 'use-package)

  (use-package combobulate
    :custom
    ;; You can customize Combobulate's key prefix here.
    ;; Note that you may have to restart Emacs for this to take effect!
    (combobulate-key-prefix "C-c o")
    :hook ((prog-mode . combobulate-mode))
    :straight (el-patch :type git :host github :repo "mickeynp/combobulate" :branch "bugfix/fix-straight"))

@tomasfarias
Copy link

Everything should work fine with emacs -q and the instructions in the readme, though?

Can confirm everything works fine with instructions in the readme. Personally, I had just added:

  (add-to-list 'load-path (expand-file-name "~/.emacs.d/straight/repos/combobulate/combobulate.el"))
  (require 'combobulate)
  (add-hook 'prog-mode-hook 'combobulate-mode)

To my config until this was resolved. Thanks for the amazing package!

The branch bugfix/fix-straight contains a potential fix.

I've tested the branch and it appears to work for me. Only just pulled the fix, but a few combobulate commands work as normal.

My config now looks like (I've commented out the lines above for testing the fix):

  (use-package combobulate
      :custom (combobulate-key-prefix "C-c b")
      :straight (:type git :host github :repo "mickeynp/combobulate" :branch "bugfix/fix-straight")
      :hook ((prog-mode . combobulate-mode)))

Then, did a quick git fetch and straight-normalize-package before testing things out in a Python file.

@mickeynp
Copy link
Owner

Hi Tomas,

Thanks for testing. I see your straight example is different from mine. I am not a straight expert. If I were to include a straight example in the README, how would you write it so it's idiomatic straight?

@tomasfarias
Copy link

Personally, I think your example looks quite the same as mine. I do also have (straight-use-package 'use-package) in my config, but it's way at the top so I omitted it from my snippet, and I do use a different prefix as I have C-c o for all org-related stuff.

The only difference I see is in your recipe having el-patch, but I don't think that's required given that the package name should match the recipe name (reading from this part of straight's readme).

So, I'd go with:

(straight-use-package 'use-package)

  (use-package combobulate
    :custom
    ;; You can customize Combobulate's key prefix here.
    ;; Note that you may have to restart Emacs for this to take effect!
    (combobulate-key-prefix "C-c o")
    :hook ((prog-mode . combobulate-mode))
    :straight (:type git :host github :repo "mickeynp/combobulate" :branch "bugfix/fix-straight"))

(Without the :branch if bugfix/fix-straight gets merged to master).

@matasar
Copy link

matasar commented Oct 18, 2024

I can confirm that the branch also works for me.

@gs-101
Copy link

gs-101 commented Oct 21, 2024

Regular (non Straight) user here. I was also having the same issue and using this branch fixed it for me. I compile Emacs from source so I'm on version 31 using the new ":vc" use-package keyword.

EDIT: Interesting, after about an hour of testing aroung my issue is a bit different now. When using the prog-mode hook, combobulate-mode becomes unusable, even when calling it with M-x. After removing the hook and restarting Emacs, no issues whatsoever, besides having to manually call combobulate-mode. This happened after I installed treesit-auto to simplify my tree-sitter configuration.

@malcolmpurvis
Copy link

I had a similar problem but I use quelpa to install the fake combobulate package.

The problem is that the generated combobulate-autoloads.elfile imports all of combobulate's files, a few of which import transient.el, which tries to create some faces from the shadow face. This face does not exist at time the autoload file is loaded. This causes the load to fail, no language support files are registered and the combobulate-mode hook skips over all files.

Strangely it only fails when running the emacs GUI (on Mac in my case). It succeeds when running Emacs from inside a terminal (emacs -nw).

The work around is to add (require 'combobulate-autoload) in your .emacs.

@mickeynp
Copy link
Owner

Weird... Nice investigation, Malcolm. I am not an expert in autoloading at all. This points to transient being the source of the issue?

@malcolmpurvis
Copy link

The problem I'm having is a result of your merging in of the bugfix/fix-straight branch. The autoload tokens that were added to combobulate.el result in the require lines being added to combobulate-autoloads.el, which results in transient.el being loaded t early at startup and failing.

It's not really the fault of transient.el. Autoload files should only contain calls to autoload, not drag in the whole implementation.

I recommend reverting that fix. The existing autoload tokens against combobulate-mode and combobulate-query-builder should be sufficient to work with any package manager.

I think that @rdeusser's original problem was caused by load-path being set to point to the combobulate.el file. It should point to the directory instead. As it was installed using straight, setting the load-path is unnecessary because it would have already done.

Your suggested config looks correct to me. My quelpa based config is similar:

(use-package combobulate
  :ensure t
  :defer t
  :quelpa (combobulate :repo "mickeynp/combobulate" :fetcher github)
  :custom
  (combobulate-key-prefix "C-c o")
  :hook ((prog-mode . combobulate-mode)))

However there remains the problem of ensuring that the language support is loaded before combobulate-mode is called. If they are not then C-c o does not get bound in any buffer. Prior to your fix they weren't loaded properly except if you used (require 'combobulate). The fix dragged them in during the autoload init phase.

I recommend moving the combobulate-mode function (with its autoload token) into combobulate.el after the require lines. That will mean that no code is loaded until its needed, but everything will be properly initialised when it is first called.

@cxa
Copy link

cxa commented Oct 24, 2024

I recommend moving the combobulate-mode function (with its autoload token) into combobulate.el after the require lines.

Confirm that works: https://github.com/cxa/combobulate

my config:

(use-package combobulate
  :vc ( :url "https://github.com/cxa/combobulate"
        :rev :newest)
  :custom (combobulate-key-prefix "C-c C-c")
  :preface
  (defun sloth/active-combobulate-on-ts-modes ()
    (when (string-match-p "-ts-mode\\'" (symbol-name major-mode))
      (combobulate-mode)))

  :hook
  (text-mode . sloth/active-combobulate-on-ts-modes)
  (prog-mode . sloth/active-combobulate-on-ts-modes))

@mickeynp
Copy link
Owner

I could reproduce the autoload of combobulate-mode issue with a clean-room configuration using straight. It -- and seemingly :vc in 30 and quelpa also? -- must do something different from the example setup in the README with an explicit load path which still works fine in Emacs 3x. I should investigate that more thoroughly at some point as that is odd.

@cxa, thank you for testing that it works. That is good to know.

I will set aside some time to fix this good and proper using your proposed solution.

Thanks again for your help, Malcolm!

@malcolmpurvis
Copy link

must do something different from the example setup in the README with an explicit load path which still works fine in Emacs 3x.

The difference is in the definition of the autoload for combobulate-mode. In all cases use-package's :hook generates the following code:

(unless (fboundp 'combobulate-mode) (autoload #'combobulate-mode "combobulate" nil t))

This is the autoload that's used in the explicit load path case. It's actually wrong because combobulate-mode is defined in combobulate-setup.el, but it happens to work because combobulate.el drags in the world and registers the language support.

If it's installed via a package manager (straight, quelpa, etc), then the package manager will generate a combobulate-autoloads.el file which includes the correct definition:

(autoload 'combobulate-mode "combobulate-setup" "..." t)

This is loaded first and because combobulate-mode is now bound, the use-package definition is skipped. However this doesn't result in registering the language support.

That's why I recommend moving the combobulate-mode function (with its autoload token) into combobulate.el because that will make both cases work.

@mickeynp
Copy link
Owner

I did not know that quelpa/straight formalised it as a package when it was retrieved through git. I figured it just checked it out and did little beyond that. That does explain a lot and qualifies why it behaves differently. Thanks!

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

No branches or pull requests

8 participants