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

lint-compile doesn't catch missing dependency on package "package" #31

Closed
jscheid opened this issue Dec 13, 2020 · 15 comments
Closed

lint-compile doesn't catch missing dependency on package "package" #31

jscheid opened this issue Dec 13, 2020 · 15 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jscheid
Copy link

jscheid commented Dec 13, 2020

When a file fails to declare a dependency on package package, lint-compile doesn't flag it. This is because when running emacs for byte-compiling, $package_initialize_file gets loaded, which asks to (require 'package) leading to a false negative.

I've worked around it like so: jscheid/prettier.el@d1f0fc1

@alphapapa
Copy link
Owner

Thanks. It seems like one could just do (unload-feature 'package) after this line:

(package-initialize)
What do you think?

BTW, note that you should probably upgrade your version of makem.sh, because there's a similar fix for byte-compilation issues in recent commits (files are now compiled separately, which reveals issues like this that could otherwise be hidden).

@jscheid
Copy link
Author

jscheid commented Dec 13, 2020

I had tried your suggestion of adding just (unload-feature 'package) first, but it broke other things:


Run SANDBOX_DIR=$(mktemp -d) || exit 1
LOG (2020-12-13 10:57:54): Initializing sandbox...
LOG (2020-12-13 10:57:54): Installing packages into sandbox...
Importing package-keyring.gpg...
Importing package-keyring.gpg...done
Contacting host: elpa.gnu.org:443
Failed to download ‘gnu’ archive.
package.el is not yet initialized!
ERROR (2020-12-13 10:57:55): Unable to initialize sandbox.

I'll try upgrading to latest to see if that works without a patch.

@jscheid
Copy link
Author

jscheid commented Dec 13, 2020

Oh, I see what you mean about compiling separately. That won't make a difference in this case because it's a single file, but yes that's a good change, I'll upgrade. Thanks for the heads up.

@alphapapa alphapapa self-assigned this Dec 13, 2020
@alphapapa alphapapa added the bug Something isn't working label Dec 13, 2020
@alphapapa alphapapa added this to the 0.4.3 milestone Dec 13, 2020
@alphapapa alphapapa modified the milestones: 0.4.3, 0.7 Aug 26, 2023
@alphapapa alphapapa modified the milestones: 0.7, 0.8 Jan 6, 2024
@alphapapa
Copy link
Owner

Not sure if this is still a problem, so deferring to 0.8. Please chime in if you can add more information.

@jscheid
Copy link
Author

jscheid commented Jan 7, 2024

@alphapapa hmm, now I can't get it to flag any packages not listed as a dependency.

For example, the below passes lint-compile and lint-package (as long as async and package-lint packages are installed).

;;; main.el --- Summary

;; Package-Requires: ((emacs "26.1"))

;;; Commentary:

;; Blah

;;; Code:

(require 'async)
(require 'package)

(provide 'main)

;;; main.el ends here

Shouldn't it complain that Package-Require doesn't include async or package?

Admittedly it's been a while since I last used makem or package-lint so I might be missing something obvious...

@alphapapa
Copy link
Owner

Shouldn't it complain that Package-Require doesn't include async or package?

Well:

  1. package is a built-in library.
  2. If you're not using --sandbox, then it will find those packages in your local Emacs configuration and not give any warnings.
  3. package-lint, which is used by the lint-package rule, is not part of makem.

@jscheid
Copy link
Author

jscheid commented Jan 7, 2024

Right, got confused here -- this ticket wasn't even about lint-package originally but about lint-compile. Ignore the above, here is the correct repro:

;;; main.el --- Summary

;;; Commentary:

;; Blah

;;; Code:

(package-version-join '(1 2 3))

(provide 'main)

;;; main.el ends here

./makem.sh lint-compile doesn't produce any warnings for it, but emacs --batch --eval '(byte-compile-file "main.el")' does:

In end of data:
main.el:9:2: Warning: the function ‘package-version-join’ is not known to be defined.

Tested with 2c4eac6 so I'd say it's still a problem.

@alphapapa
Copy link
Owner

I think this is because makem.sh loads the package library, because it needs that to load dependency packages that are installed by the package system. If you test a function from another library that's built-in but not loaded, you should get a warning, e.g.

;;; main.el --- Summary

;;; Commentary:

;; Blah

;;; Code:

(package-version-join '(1 2 3))
(lm-section-start "Commentary")

(provide 'main)

;;; main.el ends here
-*- mode: compilation; default-directory: "/tmp/makem-test/" -*-
Compilation started at Sun Jan  7 21:28:57

/tmp/makem-test/makem.sh -vvv --emacs\=emacs-29.1 --sandbox\=.sandbox lint-compile
LOG (2024-01-07 21:28:57): Initializing sandbox...
LOG (2024-01-07 21:28:57): Sandbox initialized.
LOG (2024-01-07 21:28:57): Linting compilation...
LOG (2024-01-07 21:28:57): Compiling...
LOG (2024-01-07 21:28:57): Compiling file: test.el...

In end of data:
test.el:10:2: Warning: the function `lm-section-start' is not known to be defined.
LOG (2024-01-07 21:28:57): Compiling file failed: test.el
ERROR (2024-01-07 21:28:57): Linting compilation failed.
LOG (2024-01-07 21:28:57): Finished with 1 errors.

Compilation exited abnormally with code 1 at Sun Jan  7 21:28:57

I think it's rare to call a function from package directly like this, so I'm comfortable with this being a very minor, known issue, which we could document. What do you think? Thanks.

@jscheid
Copy link
Author

jscheid commented Jan 8, 2024

@alphapapa I reported it because I did run into it in real life (jscheid/prettier.el#58), so it's rare but not just theoretical. Also, in case you've missed it, I've suggested a fix that involves unloading the package package.

That being said, I do agree it's a minor problem so if you don't want to fix it that's totally fine.

@alphapapa
Copy link
Owner

AFAICT, package is loaded even when running emacs -Q, so I don't think it would make sense to manually unload it--it would cause Emacs's state to be more different from the default. As well, that would mean that using makem.sh interactive would be unable to call package commands without manually loading the library again.

There are a number of built-in libraries which are loaded by default, and those don't require a (require FEATURE) call to be present, and it wouldn't be appropriate to unload-feature on them in order to cause a compiler warning for missing require calls that aren't needed in practice.

It appears that emacs --batch doesn't load package by default, so using --batch to byte-compile-file doesn't produce the same environment, so it emits the warning--one which shouldn't matter in practice, unless the file is actually being used in batch mode. So this appears to be an artificial problem and a false warning.

@jscheid
Copy link
Author

jscheid commented Jan 8, 2024

@alphapapa interesting, but according to the original bug report (jscheid/prettier.el#58) it seems that it can happen in an interactive setting, outside emacs --batch. Maybe something that's changed in more recent Emacs versions? I'll see if I can repro that original report on latest Emacs.

@alphapapa
Copy link
Owner

@alphapapa interesting, but according to the original bug report (jscheid/prettier.el#58) it seems that it can happen in an interactive setting, outside emacs --batch. Maybe something that's changed in more recent Emacs versions? I'll see if I can repro that original report on latest Emacs.

If package-install is configured to install packages asynchronously, it does so in a separate process, likely using --batch, which would explain the warning when installing the package, but it should still work at runtime.

@jscheid
Copy link
Author

jscheid commented Jan 8, 2024

Ahh... right, that would explain it. Good catch.

To summarize:

  • some warnings printed by package-install aren't caught by lint-compile
  • this likely only affects package package and only when package-install is configured to be async, and shouldn't have any ill effects at runtime.

I still think it's pretty easy to fix (by unloading package) but since it really is an edge case, feel free to close, up to you.

@alphapapa
Copy link
Owner

Well, the summary as I understand it something like this:

  1. Emacs loads the package library by default, except when called with --batch.
  2. package-install can be configured to install packages asynchronously, which uses --batch, which means that it byte-compiles files in a different environment than the one encountered at runtime. Usually that's not a problem, and sometimes it's even better to do it that way. makem.sh does it that way for the same reason.
  3. So calling functions from packages should work without requireing the library. But --batch doesn't "know" that, so compiling packages in it will warn if packages isn't explicitly required.

My best judgment is that unloading package would be a very bad idea; unloading any packages/libraries/features is not really supported by Emacs in the first place, despite the unload-feature command's existing. Doing so would likely lead to mysterious problems in the future. And this unusual case of a false warning caused by a built-in package being loaded automatically except for some cases in a separate Emacs process can't justify that risk.

So I'm afraid this edge case will have to remain a known issue. At least the workaround is simple on your end. :) There are many warnings that become impossible to eliminate due to supporting multiple Emacs versions, so it becomes impossible to get a clean linting sometimes.

@alphapapa alphapapa closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
@jscheid
Copy link
Author

jscheid commented Jan 9, 2024

@alphapapa OK no problem, makes sense to me. Thanks anyway for taking a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants