purcell / package-lint

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

Fix package-lint's warnings about itself #178

Closed lassik closed 4 years ago

lassik commented 4 years ago

196:65: error: You should depend on (emacs "24.4") if you need `package-desc-from-define'.

10:72: warning: An explicit dependency on cl-lib <= 1.0 is not needed on Emacs >= 24.3.

conao3 commented 4 years ago

Drop CI for Emacs-{24.1~24.3} is also reasonable? https://github.com/purcell/package-lint/blob/8edc96b5086e518be5a2feb2c705c39ddc28ff6e/.github/workflows/test.yml#L16-L18

lassik commented 4 years ago

We just discussed in #176 that package-lint needs to be able to lint packages meant for at least Emacs 24.1 and up.

Does that mean package-lint itself must also be able to run in those versions of Emacs?

Fanael commented 4 years ago

Yes, because one of the downsides of image-based platforms like Emacs is that any code checking you do will inadvertently end up depending on the current image even if you try to avoid it, just because there are so many ways such things can sneak it. The best way to be sure is to test your code using a clean image of all the Emacs versions you support, and for us this means everything all the way down to 24.1, even though we already try to minimize the way the current Emacs session influences the linting process.

lassik commented 4 years ago

That sounds reasonable. But how does package-lint ensure it can get package-desc-from-define in that case? Can we declare a manual dependency on some package.el version?

Fanael commented 4 years ago

As far as I know, there are no backports of new versions of package.el to Emacsen that have an older version built-in, so we do it the same way we do it for package-desc-summary and package-desc-name: we introduce a fallback for old Emacsen, which I have now done in commit b4eb03814f5f309a3e02c70aaa5f04b7621ff0f1.

Fanael commented 4 years ago

Closing the PR as it's no longer necessary; thanks for catching the problem, though!

purcell commented 4 years ago

we introduce a fallback for old Emacsen, which I have now done in commit b4eb038.

Thanks very much @Fanael, I had a local TODO for fixing that up and hadn't got to it. Lazy on my part.

purcell commented 4 years ago

Drop CI for Emacs-{24.1~24.3} is also reasonable?

No, for now at least, I'd like to keep package-lint running with older Emacs versions so that it can be unconditionally included in CI pipelines for packages that support old Emacsen.

conao3 commented 4 years ago

Yes, but this thread intends to change require minimum Emacs, it is reasonable on the topic. But, as you say, my comment is out of line on this package policy.

purcell commented 4 years ago

@conao3 Yep, I understood the context of your suggestion. Just wanted to clarify. :-)

lassik commented 4 years ago

Yes, but this thread intends to change require minimum Emacs

The purpose of this PR was not to change the minimum Emacs version on which package-lint runs, just to ensure that package-desc-from-define is available. Dropping support for Emacs <24.4 was the easiest way to accomplish that, but @Fanael added a shim which does the same job while keeping those Emacs versions.