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

New release of Compat #4836

Closed
minad opened this issue Jan 3, 2023 · 32 comments
Closed

New release of Compat #4836

minad opened this issue Jan 3, 2023 · 32 comments
Labels
discussions We now use the "Discussions" feature for this

Comments

@minad
Copy link
Contributor

minad commented Jan 3, 2023

Hi @tarsius,

I've taken over maintainership of Compat for now. I am working on a new version, which unfortunately breaks the API exposed by Compat. Instead of using prefixed functions, e.g., compat-string-trim, one has to call the compatibility functions with modified calling convention via compat-funcall macros. Looking up the functions is possible via compat-function.

(compat-funcall string-trim STR) ;; New
(compat-string-trim STR) ;; Old

The reason for this change is that we reduce the API surface of Compat greatly by doing that. In principle this makes it also possible to use the formerly prefixed functions in core packages, given that compat-funcall is made part of Core itself. Individual core packages could also duplicate compat-funcall.

See https://github.com/emacs-compat/compat

Please let me know if you have questions or feedback. It would be great if you could test your packages against the development version of Compat.

Daniel

@tarsius
Copy link
Member

tarsius commented Jan 3, 2023

Huhu @minad,

Thanks for taking on the responsibility to maintain this package!

(It happens, I recently ended up with EmacSQL. 🥲 )

For a moment I didn't like s/-/-funcall /, but then I looked at how many (few) times such a function is used in my code, and now I am totally fine with it.

This is already live on Melpa and GNU-devel ELPA, right? So I better hurry and make the change now?

When to you plan to merge ... ah, I see, the 29 branch has been merged. Jippie!

Guess I have to interrupt personal data hygiene week for a moment. 😀

@tarsius
Copy link
Member

tarsius commented Jan 3, 2023

(I have come across the obsolete function aliases by now.)

Given the name compat-funcall, I find it a bit strange that FUN doesn't have to be quoted, even though its symbol-name is used, not its symbol-value. Maybe "funcall" isn't the right term here?

@minad
Copy link
Contributor Author

minad commented Jan 3, 2023

For a moment I didn't like s/-/-funcall /, but then I looked at how many (few) times such a function is used in my code, and now I am totally fine with it.

Well, better now than later or never. I've discussed this change a few times with @phikal. This is the only way we can scale the package, such that it could also be used in core. Also it helps in avoiding polluting the namespace.

This is already live on Melpa and GNU-devel ELPA, right? So I better hurry and make the change now?

Only on GNU-devel ELPA. I don't think Compat is on MELPA? Otherwise I would have been more careful. (EDIT: Just checked, it is not on MELPA.)

When to you plan to merge ... ah, I see, the 29 branch has been merged. Jippie!

Philip did that earlier today. The Emacs 29 functions will be part of the next release. This should be fine I think given that the 29 branch is getting frozen soon.

Given the name compat-funcall, I find it a bit strange that FUN doesn't have to be quoted, even though its symbol-name is used, not its symbol-value. Maybe "funcall" isn't the right term here?

We have both (compat-function SYM) and (compat-funcall SYM ARGS...). Both take an unquoted symbol, which is crucial for the static lookup to work. We cannot use a variable or a quoted symbol. Do you have a suggestion for a better name?

@tarsius
Copy link
Member

tarsius commented Jan 3, 2023

We cannot use a variable or a quoted symbol.

You could take a quoted symbol and explicitly unquote it in the macro; and make an unquoted symbol a compile error.

That's one additional, "unnecessary" character, but the additional funcall is IMO more disturbing in that regard.
When I see (compat-funcall foobar ...) I think (funcall foobar ...) and go looking for where the variable foobar is bound.

Do you have a suggestion for a better name?

Cannot think of anything convincing right now. Well maybe just "call" without any of the "fun".

Actually, my first reaction to seeing the additional funcall was, "why not just compat, i.e.:

+(compat string-trim STR)
-(compat-string-trim STR)

@minad
Copy link
Contributor Author

minad commented Jan 3, 2023

You could take a quoted symbol and explicitly unquote it in the macro; and make an unquoted symbol a compile error.

Of course. That would be an extra unnecessary character. I see your argument, but these compat-funcall macros are somewhat special so I think it isn't too bad like this.

Cannot think of anything convincing right now. Well maybe just "call" without any of the "fun".

compat would be neat but I rather avoid it for now given that it is not foreseeable if we need it for something else. I want to be conservative here. compat-call would be okay I guess.

minad added a commit to emacs-compat/compat that referenced this issue Jan 3, 2023
@minad
Copy link
Contributor Author

minad commented Jan 3, 2023

Fyi on the time frame, my plan is to work a bit more on and improve the test suite. This will take maybe two weeks. If nothing problematic comes up then I will tag a new release 29.1.0.0. Any API changes we want to make should happen in this time period. There is no hurry but I also don't want to let this rest for too long.

After the release I will relatively soon remove all the prefixed compat-* functions from the development version. This shouldn't have a significant impact given that not many users pull from ELPA-devel (EDIT: Except straight users). The problem is that the prefixed compat-* functions hold development back quite a bit. I'd like to reduce the complexity of the macros which generate the compatibility functions.

In the meantime all the packages which depend on Compat have time to update. I expect this to happen quickly since the packages which use Compat are actively maintained. But there will be a longer grace period and I will make sure that all packages in the archives will be updated before I tag 29.1.1.0 without the prefix functions.

@tarsius
Copy link
Member

tarsius commented Jan 3, 2023

I would prefer the extra ', but if you go another way, I am fine with that too. I would suggest asking some others for input -- or maybe not, to avoid an endless discussion on a minor detail. 😁

@tarsius
Copy link
Member

tarsius commented Jan 3, 2023

Ups... messages crossed. compat-call it is then.

@tarsius
Copy link
Member

tarsius commented Jan 3, 2023

Plan sounds good too.

tarsius added a commit that referenced this issue Jan 3, 2023
@minad
Copy link
Contributor Author

minad commented Jan 4, 2023

@tarsius I just checked MELPA and ELPA. It seems that you and I are the only Compat user who uses these prefixed compat-* functions. Furthermore I went over the entire test suite, fixed bugs and improved coverage. If you find time, there are two things I'd like you to ask:

  1. Could you please test your packages (mainly Magit) against the Compat master branch? In Compat we use Purcell's workflows configuration to test against all relevant Emacs versions: https://github.com/emacs-compat/compat/actions/runs/3841315012/jobs/6541388156. If you could check that Magit works with Compat master it would be greatly appreciated.

  2. As soon as you feel ready about the update please let me know. I have the hope that things will go smoothly, in particular if your CI run went well. Nevertheless there is a small risk that things will break for users. I've seen angry users appearing before on the issue trackers, so we should at least be prepared for impact. ;)

@Lenbok
Copy link

Lenbok commented Jan 4, 2023

I (presumably through bad timing in my straight updates) found that magit is now breaking when trying to call compat--internal-assoc (or something similar, I no longer have the message). Switching to the magit compat test branch seemed to fix things.

@minad
Copy link
Contributor Author

minad commented Jan 4, 2023

@Lenbok Ah right, I forgot about Straight users. They will obviously be affected by any changes since they pull directly from Git. This means that we get many beta testers at least. :)

Which branch do you mean? The compat-call Magit branch (https://github.com/magit/magit/tree/compat-call)? If you have time it would be great if you could test Compat master (https://github.com/emacs-compat/compat/tree/master) against both the Magit master and the compat-call branch.

@hexmode
Copy link

hexmode commented Jan 4, 2023

I'm getting problems today with compat--internal-assoc. I'm using straight and the repo for compat.el is github.com/emacs-straight/compat.git

Is there a way to override that? I guess adding

(use-package compat
  :straight (:host github :repo "emacs-compat/compat"))

would do it.

@minad
Copy link
Contributor Author

minad commented Jan 4, 2023

@hexmode Yes, pulling directly from upstream should fix the issue. But the straight mirror should also synchronize again soon.

@hexmode
Copy link

hexmode commented Jan 4, 2023

I also had to add compat to straight-recipe-overrides. But it seems to be working.

@emacs18
Copy link
Contributor

emacs18 commented Jan 5, 2023

Same here. I ran into the same problem after I built fresh emacs 29, latest spacemacs, and latest git versions for all packages.
Adding the following in my early-init.el resolved the problem for me as well.

(straight-override-recipe '(compat :host github :repo "emacs-compat/compat"))

@minad
Copy link
Contributor Author

minad commented Jan 5, 2023

@emacs18 I wonder why. In the meantime, the emacs-straight mirror has updated to a version which should work (emacs-straight/compat@46d9764). Can you please check if the override is still necessary? It should not be!

tarsius added a commit that referenced this issue Jan 5, 2023
tarsius added a commit that referenced this issue Jan 5, 2023
@tarsius tarsius closed this as completed in 108ed15 Jan 5, 2023
@tarsius
Copy link
Member

tarsius commented Jan 5, 2023

I've updated my packages. I had difficulties confirming that I've updated all of them because rgrep was broken. I haven't looked at compat for quite a while and am not sure how things are supposed to work anymore, but I am under the impression that there is a regression.

The error I get is about string-lines being called with three arguments instead of one or two. Turns out the definition from compat-28.el is being used, but I am on Emacs 29. That doesn't seem right. I did recompile everything multiple times.

(Also, is it a bug that compat's definition of that function doesn't take an optional third argument?)

@minad
Copy link
Contributor Author

minad commented Jan 5, 2023

@tarsius

The error I get is about string-lines being called with three arguments instead of one or two. Turns out the definition from compat-28.el is being used, but I am on Emacs 29. That doesn't seem right. I did recompile everything multiple times.

Thanks, there was indeed a bug, which is fixed now. See emacs-compat/compat@06d0369.

(Also, is it a bug that compat's definition of that function doesn't take an optional third argument?)

No, that is not a bug of Compat. Compat does not yet provide a new version of string-lines. If you want to use two arguments you can obviously do that on every Emacs version. Only on Emacs 29 (and newer) you can use three arguments. If we add support for three arguments in Compat, then you would have to call it via (compat-call string-lines STRING OMIT-NULLS KEEP-NEWLINES), which would then work on every Compat-supported Emacs version.

Note that Emacs 29 support is not yet complete. We can add more functionality on a case by case basis if needed. The most important functions are there though, and they are tested.

@tsdh
Copy link
Contributor

tsdh commented Jan 6, 2023

@minad @tarsius
Hi guys, I'm left with broken magit and consult packages where stuff like (compat-call executable-find ...) errors that executable-finds symbol value as variable is void. I've fixed it locally (removed the compat-call). Is that just a temporary hiccup until new melpa versions are built?

@minad
Copy link
Contributor Author

minad commented Jan 6, 2023

@tsdh Can you check again, update and reinstall both Compat and Magit? It seems you updated Magit but not Compat. Which method do you use to install packages?

@tsdh
Copy link
Contributor

tsdh commented Jan 6, 2023

@minad Ah, now I've got the compat update. I use use-package with :ensure t for all packages and run emacs -batch -l ~/.emacs.d/init.el -f package-update-all every morning to fetch updates. I guess Magit and Consult might have had a new release before compat?

Anyway, now after receiving the compat update and reinstalling Magit and Consult, everything seems to work again.

@minad
Copy link
Contributor Author

minad commented Jan 6, 2023

@tsdh Thank you for confirming. Sorry for the hiccup. I just checked Magit and everything seems alright there. compat-call is a new macro provided by compat.

  1. The macro is used in magit-autorevert.
  2. magit-autorevert requires magit-git.
  3. magit-git requires magit-base.
  4. magit-base requires compat, which should make the macro available and we are alright!

This means you had a miscompilation. I wonder why Magit was happily updated/compiled on your system, while the Compat dependency wasn't satisfied.

@tsdh
Copy link
Contributor

tsdh commented Jan 6, 2023

I don't know either and sadly there's not package update log (AFAICT). Anyway, everything works again. :-)

@minad
Copy link
Contributor Author

minad commented Jan 6, 2023

Btw, @tarsius, it should not be necessary anymore to require explicitly versioned compat libraries since 29.1.0.0. (require compat) is all that is needed now:

magit/lisp/magit-base.el

Lines 40 to 41 in b281f05

(require 'compat-26)
(require 'compat-27)

(The unnecessary requires won't do any harm, they are just unnecessary.)

@tarsius
Copy link
Member

tarsius commented Jan 6, 2023

Reopening for the benefit of those running into issues.

@kaushalmodi
Copy link

Hello, I am using the latest Magit version

Magit 20230107.2134 [>= 3.3.0.50-git], Git 2.37.0, Emacs 29.0.60, gnu/linux

on Emacs built using the latest emacs-29 branch.

After the recent update of Magit, I am seeing this error and I believe it has to do with the recent change of compat version?

Debugger entered--Lisp error: (void-function compat-assoc)
  compat-assoc(((branch . #("origin/main" 0 11 (face magit-branch-remote font-lock-face magit-branch-remote))) (branch . "main") (status)) nil equal)
  magit-section-cache-visibility(#<magit-section "origin/main" [branch branch status] 122-188>)
  magit-section-maybe-cache-visibility(#<magit-section "origin/main" [branch branch status] 122-188>)
  magit-section-hide(#<magit-section "origin/main" [branch branch status] 122-188>)
  magit-section-show-children-1(#<magit-section "main" [branch status] 37-[122-]277> 0)
  magit-section-show-children-1(#<magit-section nil [status] 1-19828> 1)
  magit-section-show-children(#<magit-section nil [status] 1-19828> 1)
  magit-section-show-level(-2)
  magit-section-show-level-2-all()
  funcall-interactively(magit-section-show-level-2-all)
  call-interactively(magit-section-show-level-2-all nil nil)
  command-execute(magit-section-show-level-2-all)

@kaushalmodi
Copy link

Hmm, uninstalling Magit (and all its dependencies) and reinstalling Magit fixed the above issue for me. It seems like I ended up with mixed installation of magit-section somehow.

@tarsius
Copy link
Member

tarsius commented Jan 11, 2023

@Spike-Leung
Copy link

Hmm, uninstalling Magit (and all its dependencies) and reinstalling Magit fixed the above issue for me. It seems like I ended up with mixed installation of magit-section somehow.

Thanks :)
Uninstalling and reinstalling magit does not work for me, after manually removing the old magit-section and restarting emacs then it works.

@nbarrientos
Copy link
Contributor

Just wanted to add that I also ran into this (broken calls to (compat-call executable-find ...) in magit-turn-on-auto-revert-mode-if-desired). The only way out was to upgrade compat and clean up the magit installation (packages, natively compiled files etc).

@hrehfeld
Copy link

hrehfeld commented Mar 20, 2023

edit: ignore for now, not sure if maybe my magit didn't update correctly before. I'm gonna see if it happens again.

Should be related, is this the right place? I think this happens after a commit:

Debugger entered--Lisp error: (void-function compat-assoc-delete-all)
  compat-assoc-delete-all(this-commit-command ("<git-repo>" (this-commit-command . magit-commit-create) (mode-line-process . #(" git commit" 1 11 (font-lock-face magit-mode-line-process help-echo "mouse-1: Show process buffer" keymap (keymap (mode-line keymap (mouse-1 . magit-process-buffer))) mouse-face highlight)))))
  magit-repository-local-delete(this-commit-command)
  magit-commit--reset-command()
  with-editor-finish(nil)
  funcall-interactively(with-editor-finish nil)
  call-interactively(with-editor-finish nil nil)
  command-execute(with-editor-finish)

After I did straight-pull-all earlier, emacs 29.0.60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussions We now use the "Discussions" feature for this
Projects
None yet
Development

No branches or pull requests

10 participants