leotaku/elisp-check

workflow config?

Closed this issue · 5 comments

bdarcus commented

I've been getting errors when using this recently that I don't understand.

Here's the latest run:

https://github.com/bdarcus/bibtex-actions/pull/260/checks?check_run_id=3432408741

Issues:

  1. it's complaining about not being able to load a file on oc-bibtex-actions.el; but I do not include that file in the config (so I can avoid this error).
  2. often, though not on this run, I get errors about not being able to load a dependency
  3. a more minor issue: it's issuing a warning about an unknown "string-search" function, but that's built in
  4. it complaints about package-requires being ignored on non-main files, but if I remove the line from the header, it complains about there not being a package-requires (because of lexical binding)

The weird thing is I get no complaints from package-lint or checkdoc locally.

Maybe this just isn't well-suited to multi-file packages?

leotaku commented
  1. I see that you do not specify the main file that elisp-check should run for in your CI config. If you do not do this, elisp-check will run for all Elisp files in the root directory. I now think that this was a bad choice as it can lead to confusion, but fixing it now would be a backwards-incompatible change. I would recommend you switch to something like what is shown here with using a workflow matrix for the desired entry files and report back to me if the issue persists. For this specific issue, I would recommend adding the package that provides oc (I assume org-cite?) here, maybe that fixes it.
  2. I'm not sure if this is actually it, but if this occurs seemingly at random, it could be a problem with the reliability of MELPA or ELPA. I could probably implement something like an retry mechanism with exponential backoff, but before I do that I would like to confirm that this is actually the issue. If possible, send me a workflow run where this issue is present.
  3. No clue, but I'll check later when I have access to a PC.
  4. Right, I think this is just an inconsistency with Emacs packaging best practices. If you just require another file for purposes of code organisation, that file should generally not have a Package-Requires header. If that file actually makes sense as a separate package, it should of course contain a Package-Requires header. Now the issue arises with files that make sense as their own package, but are also required by some larger package. I would keep the Package-Requires header and just remember that the warning is incorrect.

Maybe this just isn't well-suited to multi-file packages?

Elisp-check is supposed to work well with multi-file packages, but there isn't really a standard for how they should work, so some things can be unintuitive.

bdarcus commented

Thanks!

Following your recommendation on the matrix files seems to fix the issues, though I haven't yet gotten to try out ...

I would recommend adding the package that provides oc (I assume org-cite?) here, maybe that fixes it.

Yes, this is org-cite, which is included in 9.5; oc IS the org-cite module, though originally this file listed both symbols.

If you just require another file for purposes of code organisation, that file should generally not have a Package-Requires header. If that file actually makes sense as a separate package, it should of course contain a Package-Requires header.

I guess the oc-bibtex-actions.el file is a bit in-between. I haven't yet been bothered to do a separate package for MELPA, but I do need to make clear it will only work with org 9.5; e.g with the oc module available.

Thanks for your help, BTW!

leotaku commented

Sure thing!

Addendum to 3., according to my local Emacs help string-search was introduced in version 28.1. So the warning about the function being undefined should actually be correct. Notice how your workflow runs using Emacs version snapshot don't have this warning.

Yes, this is org-cite, which is included in 9.5; oc IS the org-cite module, though originally this file listed both symbols.

Right, I wasn't aware that org-cite was actually part of org-mode. I'll have to take a look at the new org features sometime. :)

I guess the oc-bibtex-actions.el file is a bit in-between. I haven't yet been bothered to do a separate package for MELPA, but I do need to make clear it will only work with org 9.5; e.g with the oc module available.

I agree, but I also don't really think you need or even should make a separate package. My thinking was more just that, because the file provides actual separate functionality and doesn't just exist for the purposes of code organization, it might make sense to think of it as a separate package.

leotaku commented

@bdarcus Hey, just checking in to confirm whether you have been able to resolve this. If you have, I'd appreciate it if you could close this issue.

bdarcus commented

I haven't had the time to confirm, but I'll close for now. Thanks again.