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

Declare version with "Version" rather than "Package-Version" #264

Closed
wants to merge 1 commit into from

Conversation

aagon
Copy link

@aagon aagon commented Nov 26, 2023

Hello,

I assume this is the place to report this, rather than the emacs mailing list.

Since transient is now a builtin package of emacs, the variable package--builtins in the finder-inf library stores its name, version and a short description :

...
(tramp . [(2 6 0 29 1) nil "Transparent Remote Access, Multiple Protocol"])
(transient . [nil nil "Transient commands"])
(tree-widget . [nil nil "Tree widget"])
...

The version is not reported properly since the header keyword used to declare the version in transient is Package-Version, which is package-specific, rather than Version, which is the standard way (finder, the library that sets package--builtins, looks for this header only).

package looks for both Package-Version and Version headers, so this should not break anything.

Best,

Aymeric Agon-Rambosson

@tarsius
Copy link
Member

tarsius commented Nov 26, 2023

Thanks!

Ah great, consistency going out the window. (I use Package-Version in all my packages; long story.)

Please add the reason why we want this to the commit message as well. (Because I don't actually want this. I am forced to do it and might forget why.)

@aagon
Copy link
Author

aagon commented Nov 26, 2023

Please add the reason why we want this to the commit message as well.

Done

I am forced to do it

Sorry for the bother. I could also have tried to change finder-compile-keywords directly using the mailing list, but this would have been probably refused. To give you an idea, among all the builtin elisp packages, only transient and emoji use "Package-Version" rather than "Version".

@tarsius
Copy link
Member

tarsius commented Nov 27, 2023

I cannot wrap my head around these errors (or rather raised warnings).

Firstly it's not true, these variables are lexically bound. And this code has existed for a while without this happening and I just pushed other changes that also include this code without this happening either. But I rebased and pushed your branch and it is still happening, so it wasn't just a flux. And yet, your change seems completely unrelated and my CI scripts don't seem to care about either of these two variables. Can you explain it?

Emacs builtin packages are supposed to specify their version using the header
"Version" for their version to be correctly stored in the variable
package--builtins.

This is because the function finder-compile-keywords, responsible for setting
this variable, parses the source files looking for this exact header.

This does not break package, since the function package-buffer-info looks for
both Package-Version and Version.
@tarsius
Copy link
Member

tarsius commented Nov 27, 2023

I've started using static-if, which makes the warnings go away.

The version in Emacs already differs from what we have here (see ed78d3b), so I'll only switch to this header in Emacs (I'm already doing the same for URL/Homepage.)

@aagon
Copy link
Author

aagon commented Nov 27, 2023

The version in Emacs already differs from what we have here (see ed78d3b), so I'll only switch to this header in Emacs (I'm already doing the same for URL/Homepage.)

You mean this change should be part of commit ed78d3b ? This would certainly make sense to bundle it with all the other changes you must do when moving from here to emacs.

This would require amending ed78d3b. I don't think I can do this with a PR though.

@tarsius
Copy link
Member

tarsius commented Nov 27, 2023

Yes. I'll do this myself (I also have to make other changes.)

@tarsius
Copy link
Member

tarsius commented Dec 5, 2023

This has now landed in Emacs' master branch.

There was some turbulence. 🥴

@tarsius tarsius closed this Dec 5, 2023
@tarsius
Copy link
Member

tarsius commented Dec 5, 2023

Thanks for bringing this to my attention!

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

Successfully merging this pull request may close these issues.

2 participants