purcell / package-lint

A linting library for elisp package metadata
GNU General Public License v3.0
194 stars 34 forks source link

Suggestion: Don't check Package-Version #275

Closed meedstrom closed 3 weeks ago

meedstrom commented 4 weeks ago

I propose that package-lint--check-package-version-present should no longer fire, at least by default

Only the GNU/NonGNU Elpa repositories require a Package-Version header, and in Melpa there is a case to be made that the absence is not a bug but a feature, reducing the incidence of mistakes. If emacs-devel wants to see the header, they can probably detect the absence themselves.

It's not a big deal, of course, since this only counts as a warning and not an error. However the sheer number of warnings I have had to ask for exceptions from is getting pretty high, and at some point the MELPA maintainers may go by feel of the number of warnings excepted-so-far. Related: https://github.com/melpa/melpa/pull/9133#issuecomment-2336801635

Besides. Warnings have a negative connotation and so it slows community adoption of the method of using git/hg tags instead. Even if I were neutral on that matter, I would not offer such an opinion to every package author. Neutrality would be downgrading it to an informational message, not a warning, or more simply just leaving it out.

purcell commented 3 weeks ago

Responded here:

At least for single file packages, you can't install them without a Package-Version header.

meedstrom commented 3 weeks ago

Alright, I would regard that a bug in package.el, because it breaks what needn't be broken. I'll probably write to bug-gnu-emacs later.

I have a reservation about adding a ;; Package-Version: 0, which is that I do not know if new package managers like Elpaca might not then favour that over the git/hg tag, once it is present. Omitting it sends a clear message.

I suppose I could go look up how these package managers actually work, but a new package manager could always appear, and it is strange that every package has to add a non-semantic line with a fake version in the first place to prevent a specific command package-install-file from breaking.

meedstrom commented 3 weeks ago

I surmise that multi-file packages work because MELPA builds generate a -pkg.el file? If so, maybe it can also insert a ;; Package-Version: 0 in the single-file packages :)

And then the way is clear to implement this suggestion. (Still need to write to bug-gnu-emacs, of course, anyway)

purcell commented 3 weeks ago

Alright, I would regard that a bug in package.el, because it breaks what needn't be broken. I'll probably write to bug-gnu-emacs later.

I don't think it's a bug, though. It's reasonable (and common in other ecosystems, e.g. debian packages) to install a package from a file that has been obtained separately from some source, and for that file to be self-describing, including containing a version. For single-file packages, Package-Version is where that version is placed, and that doesn't seem like a bad thing.

I surmise that multi-file packages work because MELPA builds generate a -pkg.el file? If so, maybe it can also insert a ;; Package-Version: 0 in the single-file packages :)

MELPA already inserts the (correct) version into the Package-Version header of single-file packages. Again, because they always were (and still are?) ultimately installed via package-install-file, which requires that metadata.

meedstrom commented 3 weeks ago

Yes, I suppose it is only a bug in some sense (the simple sense of breaking what would have otherwise worked, but there's more to a bug than that), perhaps I should not throw around the word so lightly.

MELPA already inserts the (correct) version

That's great, and even better than inserting a Version 0!

I think if the repo has tags, it is self-describing, no file needs to be, but yea, it makes sense that when pulling files out of the repo to make a release, a string-substitution is performed somewhere. [1]

Since MELPA is already doing this, I guess any problems would mainly be encountered when someone attempts package-install-file manually on a manually cloned repo. I take it that you have opted against just... leaving it to package-install-file to print the error in that situation?

I'd be a bit more iconoclastic, but that has pros and cons. You can close the issue. :)

[1] This has actually enlightened me a bit on what Straight/Elpaca do not/cannot do, so thanks! Currently Elpaca is in a complicated situation because it wants to warn about mismatched dependencies, but also default to doing a shallow clone, in which the tag information is gone, and since the cloned repo is something the user is supposed to be able to develop inside, it can do no string substitutions. My workaround now as a packager is to always keep a tag on the distribution branch.

I wonder if Git can include the tag history even without including the commits... maybe a "LAST_TAG" file can just be inserted into .git/...

purcell commented 3 weeks ago

I take it that you have opted against just... leaving it to package-install-file to print the error in that situation?

Yeahm but the point of package-lint is to show potential issues with how the code is packaged, and particularly issues that would prevent installation.

I can't comment much on the Elpaca workflow. For packages I'm developing, I work on them in separate clones, and rarely try to use that local in-development version of a package as my daily driver.

FWIW, MELPA also uses shallow clones, but for MELPA Stable we grab all the tags, determine the highest-numbered version tag, and then check that out.

meedstrom commented 3 weeks ago

FWIW, MELPA also uses shallow clones, but for MELPA Stable we grab all the tags, determine the highest-numbered version tag, and then check that out.

This requires cloning the full repo first though, right? Pardon, I haven't tried it.

meedstrom commented 3 weeks ago

Ah, I see, there is a git ls-remote thing!

purcell commented 3 weeks ago

Oh nice. I actually think we clone the full repo in that case, but we can perhaps streamline it.

meedstrom commented 3 weeks ago

In case you want to see an example, I just did a PR to Elpaca https://github.com/progfolio/elpaca/pull/359.

Rewriting in self-contained vanilla Elisp, it looks like this:

(with-temp-buffer
  (if (zerop (call-process "git" nil t nil
                           "-c" "versionsort.suffix=-" ;; https://git-scm.com/docs/git-config#Documentation/git-config.txt-versionsortsuffix
                           "ls-remote" "--refs" "--tags"
                           "--exit-code" ;; https://git-scm.com/docs/git-ls-remote.html#Documentation/git-ls-remote.txt---exit-code
                           "--sort=-version:refname" ;; https://git-scm.com/docs/git-tag#Documentation/git-tag.txt---sortltkeygt
                           "origin"))
      (mapcar (lambda (line)
                (thread-first line
                              (split-string "\t" t)
                              (cadr)
                              (split-string "/" t)
                              (caddr)))
              (split-string (buffer-string) "\n" t))
    (message "Git failed with output:\n %s" (buffer-string))))

and if you eval that in a directory that is a git repo, you should get the version-sorted list of tags, even if that repo is shallow.

alphapapa commented 3 weeks ago

FWIW, note that the Elisp manual says:

   The version number comes from the ‘Package-Version’ header, if it
exists, or from the ‘Version’ header otherwise.  One or the other _must_
be present.

I was initially confused since I've always used Version: rather than Package-Version:.

And FWIW, I would not like this check to be disabled by default, because then I'd have to configure all of my packages to specifically enable it (and invent a way for makem.sh to pass such an option through to package-lint).

@meedstrom When considering changes to widely used tools, it's very important to consider what effects the changes could have on downstream users and tools and whether such changes are really necessary. Sometimes something as innocuous as a change in indentation causes unexpected grief, which time it takes to deal with may be multiplied by the number of downstream users. Even worse is when the tool in question tends to be used in CI pipelines where the latest version is used automatically, without the downstream users' having a chance to read changelogs and make changes for compatibility.

meedstrom commented 3 weeks ago

And FWIW, I would not like this check to be disabled by default, because then I'd have to configure all of my packages to specifically enable it

You mean your initfiles, right? At least, I can't see putting a package-lint setting in dir-locals.el when one could just insert the Version: header and then that's it for that package, permanently.

As for makem.sh, I understand your concern about downstream users. But can you describe a situation where it would cause grief? It is the removal of a message, not introduction of a CI-stopping step. I'll buy that the removal of an error can feasibly break CIs, but this was never an error.

alphapapa commented 2 weeks ago

@meedstrom As a final suggestion, I'd suggest that we both remove our comments here, since they are more like a personal conversation than for public consumption. I'll remove mine now.

purcell commented 2 weeks ago

Catching up on the emails, I know what you guys spent your weekend on. 😂 Hope you both have a great week.

meedstrom commented 2 weeks ago

Ahh, the emails do not include my edits. I fear my posts will be more rude than I endorse. Fair warning!

purcell commented 2 weeks ago

Don't worry, I'm gonna move on with my day and have another coffee instead.

alphapapa commented 2 weeks ago

Don't worry, I'm gonna move on with my day and have another coffee instead.

Now that's good advice. ;) Thanks, Steve.