bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Antonio Romano via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Flycheck includes the "flycheck-emacs-lisp-initialize-packages"
customizable variable, which allows packages to be initialized in the
Emacs subprocess used to lint the elisp file. With this variable, it is
possible for the linter to be aware of autoloads from installed
packages.

I think that having such option on the built-in alternative "flymake"
would be a good quality-of-life addition, as this would make it more
suitable for linting personal configuration and only one M-x away from
any user who would want to try it.

Thanks in advance

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Pengji Zhang]

Hi!

I am switching to Flymake and I missed this feature as well. I think
this bug report is related:

    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48452

In that thread João gave a patch that implements this feature (sort of):

    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48452#43

Although it was not installed.

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Eli Zaretskii]

> From: Pengji Zhang <me@pengjiz.com>
> Date: Wed, 23 Oct 2024 17:01:23 +0800
> 
> I am switching to Flymake and I missed this feature as well. I think
> this bug report is related:
> 
>     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48452
> 
> In that thread João gave a patch that implements this feature (sort of):
> 
>     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48452#43
> 
> Although it was not installed.

Adding Spencer, who nowadays maintains Flymake, and João.

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[João Távora]

On Wed, Oct 23, 2024 at 11:33 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Pengji Zhang <me@pengjiz.com>
> > Date: Wed, 23 Oct 2024 17:01:23 +0800
> >
> > I am switching to Flymake and I missed this feature as well. I think
> > this bug report is related:
> >
> >     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48452
> >
> > In that thread João gave a patch that implements this feature (sort of):
> >
> >     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48452#43
> >
> > Although it was not installed.
>
> Adding Spencer, who nowadays maintains Flymake, and João.

Feel free to use my patch of course.  Stefan noted some time ago
that -f package-initialize is the wrong flag to ask Emacs -Q to use
elpa though, so there's something better.

João

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Pengji Zhang]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

João Távora <joaotavora@gmail.com> writes:

> Feel free to use my patch of course.  Stefan noted some time ago
> that -f package-initialize is the wrong flag to ask Emacs -Q to use
> elpa though, so there's something better.

Thanks! I prepared a new patch based on yours.

Regards,
Pengji


[-- Attachment #2: 0001-Add-option-elisp-flymake-byte-compile-activate-packa.patch --]
[-- Type: text/x-patch, Size: 5034 bytes --]

From be772ff2f3bae82977d6c54fa84960f721ba088e Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Fri, 25 Oct 2024 19:44:44 +0800
Subject: [PATCH] Add option 'elisp-flymake-byte-compile-activate-packages'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This option controls whether the Flymake Emacs Lisp
byte-compiler should activate user installed packages before
checking the source buffer.  (Bug#65035)

* lisp/progmodes/elisp-mode.el
(elisp-flymake-byte-compile-user-file-p): New predicate function
to check if a buffer is visiting a user file.
(elisp-flymake-byte-compile-activate-packages): New option.
(elisp-flymake--byte-compile-activate-packages): New variable
for caching.
(elisp-flymake-byte-compile): Use the new option.

* etc/NEWS: Announce the new option.

Co-authored-by: João Távora <joaotavora@gmail.com>
---
 etc/NEWS                     |  7 ++++++
 lisp/progmodes/elisp-mode.el | 41 ++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index a6c2c895985..090f4293c8e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -413,6 +413,13 @@ This affects calls to 'warn', 'lwarn', 'display-warning', and
 In most cases, having it enabled leads to a large amount of false
 positives.
 
+---
+*** New user option 'elisp-flymake-byte-compile-activate-packages'.
+This option controls whether or not the Flymake byte-compiler backend
+should activate user installed packages before compiling the source
+buffer.  By default, it is set to activate packages when checking user
+configuration files.  Set it to nil to restore the previous behavior.
+
 ** DocView
 
 ---
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 2f931daedc7..8a9dce5d3f6 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -2190,6 +2190,39 @@ elisp-flymake-byte-compile-load-path
 
 (defvar bytecomp--inhibit-lexical-cookie-warning)
 
+(defun elisp-flymake-byte-compile-user-file-p (buffer)
+  "Return non-nil if BUFFER is visiting a user file.
+That means either the file is `user-init-file' or it is in
+`user-emacs-directory'."
+  (when-let* ((file (buffer-local-value 'buffer-file-truename buffer)))
+    (or (and user-emacs-directory
+             (file-in-directory-p file user-emacs-directory))
+        (and user-init-file
+             (string= file (abbreviate-file-name
+                            (file-truename user-init-file)))))))
+
+(defcustom elisp-flymake-byte-compile-activate-packages
+  #'elisp-flymake-byte-compile-user-file-p
+  "Whether to activate packages for Flymake elisp byte-compilation.
+If the value is nil, do not activate installed packages.  If the value
+is a function, it is called with one argument, the source buffer to be
+checked, and installed packages are activated if the function returns
+non-nil.  Otherwise, packages are always activated.
+
+Note that for efficiency the return value of the predicate function is
+cached the first time it is called.  Type \\[revert-buffer-quick] to
+invalidate the cached value."
+  :type '(choice
+          (const :tag "Don't activate" nil)
+          (const :tag "Always activate" t)
+          (const :tag "Activate for user files"
+                 elisp-flymake-byte-compile-user-file-p)
+          (function :tag "Predicate function"))
+  :group 'lisp)
+
+(defvar-local elisp-flymake--byte-compile-activate-packages :unset
+  "Cached value for `elisp-flymake-byte-compile-activate-packages'.")
+
 ;;;###autoload
 (defun elisp-flymake-byte-compile (report-fn &rest _args)
   "A Flymake backend for elisp byte compilation.
@@ -2205,6 +2238,12 @@ elisp-flymake-byte-compile
     (save-restriction
       (widen)
       (write-region (point-min) (point-max) temp-file nil 'nomessage))
+    (when (eq elisp-flymake--byte-compile-activate-packages :unset)
+      (setq elisp-flymake--byte-compile-activate-packages
+            (if (functionp elisp-flymake-byte-compile-activate-packages)
+                (funcall elisp-flymake-byte-compile-activate-packages
+                         source-buffer)
+              elisp-flymake-byte-compile-activate-packages)))
     (let* ((output-buffer (generate-new-buffer " *elisp-flymake-byte-compile*"))
            ;; Hack: suppress warning about missing lexical cookie in
            ;; *scratch* buffers.
@@ -2223,6 +2262,8 @@ elisp-flymake-byte-compile
                    ;; "--eval" "(setq load-prefer-newer t)" ; for testing
                    ,@(mapcan (lambda (path) (list "-L" path))
                              elisp-flymake-byte-compile-load-path)
+                   ,@(when elisp-flymake--byte-compile-activate-packages
+                       '("-f" "package-activate-all"))
                    ,@warning-suppression-opt
                    "-f" "elisp-flymake--batch-compile-for-flymake"
                    ,temp-file)
-- 
2.47.0

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Eli Zaretskii]

> From: Pengji Zhang <me@pengjiz.com>
> Cc: sbaugh@janestreet.com, João Távora
>  <joaotavora@gmail.com>, Eli Zaretskii
>  <eliz@gnu.org>
> Date: Fri, 25 Oct 2024 19:50:48 +0800
> 
> João Távora <joaotavora@gmail.com> writes:
> 
> > Feel free to use my patch of course.  Stefan noted some time ago
> > that -f package-initialize is the wrong flag to ask Emacs -Q to use
> > elpa though, so there's something better.
> 
> Thanks! I prepared a new patch based on yours.

Thanks.  Spencer, any comments?

> +(defcustom elisp-flymake-byte-compile-activate-packages
> +  #'elisp-flymake-byte-compile-user-file-p
> +  "Whether to activate packages for Flymake elisp byte-compilation.
> +If the value is nil, do not activate installed packages.  If the value
> +is a function, it is called with one argument, the source buffer to be
> +checked, and installed packages are activated if the function returns
> +non-nil.  Otherwise, packages are always activated.
> +
> +Note that for efficiency the return value of the predicate function is
> +cached the first time it is called.  Type \\[revert-buffer-quick] to
> +invalidate the cached value."

I think the doc string should explicitly mention
elisp-flymake-byte-compile-user-file-p, since it is used as the
default value of the option.

> +  :type '(choice
> +          (const :tag "Don't activate" nil)
> +          (const :tag "Always activate" t)
> +          (const :tag "Activate for user files"
> +                 elisp-flymake-byte-compile-user-file-p)
> +          (function :tag "Predicate function"))
> +  :group 'lisp)

Please add a :version tag here.

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Pengji Zhang]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> I think the doc string should explicitly mention
> elisp-flymake-byte-compile-user-file-p, since it is used as the
> default value of the option.
>
> [...]
>
> Please add a :version tag here.

Thanks for the review! Fixed in the attached updated patch.

Pengji


[-- Attachment #2: 0001-Add-option-elisp-flymake-byte-compile-activate-packa.patch --]
[-- Type: text/x-patch, Size: 5232 bytes --]

From 6d34dcc9de1f99c795d0a6bcaa416e29c8500eed Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Sun, 10 Nov 2024 09:30:01 +0800
Subject: [PATCH] Add option 'elisp-flymake-byte-compile-activate-packages'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This option controls whether the Flymake Emacs Lisp
byte-compiler should activate user installed packages before
checking the source buffer.  (Bug#65035)

* lisp/progmodes/elisp-mode.el
(elisp-flymake-byte-compile-user-file-p): New predicate function
to check if a buffer is visiting a user file.
(elisp-flymake-byte-compile-activate-packages): New option.
(elisp-flymake--byte-compile-activate-packages): New variable
for caching.
(elisp-flymake-byte-compile): Use the new option.

* etc/NEWS: Announce the new option.

Co-authored-by: João Távora <joaotavora@gmail.com>
---
 etc/NEWS                     |  7 ++++++
 lisp/progmodes/elisp-mode.el | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index a6c2c895985..090f4293c8e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -413,6 +413,13 @@ This affects calls to 'warn', 'lwarn', 'display-warning', and
 In most cases, having it enabled leads to a large amount of false
 positives.
 
+---
+*** New user option 'elisp-flymake-byte-compile-activate-packages'.
+This option controls whether or not the Flymake byte-compiler backend
+should activate user installed packages before compiling the source
+buffer.  By default, it is set to activate packages when checking user
+configuration files.  Set it to nil to restore the previous behavior.
+
 ** DocView
 
 ---
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 2f931daedc7..3905300c9a8 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -2190,6 +2190,44 @@ elisp-flymake-byte-compile-load-path
 
 (defvar bytecomp--inhibit-lexical-cookie-warning)
 
+(defun elisp-flymake-byte-compile-user-file-p (buffer)
+  "Return non-nil if BUFFER is visiting a user file.
+That means either the file is `user-init-file' or it is in
+`user-emacs-directory'."
+  (when-let* ((file (buffer-local-value 'buffer-file-truename buffer)))
+    (or (and user-emacs-directory
+             (file-in-directory-p file user-emacs-directory))
+        (and user-init-file
+             (string= file (abbreviate-file-name
+                            (file-truename user-init-file)))))))
+
+(defcustom elisp-flymake-byte-compile-activate-packages
+  #'elisp-flymake-byte-compile-user-file-p
+  "Whether to activate packages for Flymake elisp byte-compilation.
+If the value is nil, do not activate installed packages.  If the value
+is a function, it is called with one argument, the source buffer to be
+checked, and installed packages are activated if the function returns
+non-nil.  Otherwise, packages are always activated.
+
+The default value is a predicate function
+`elisp-flymake-byte-compile-user-file-p' (which see), and that means
+packages are activated only for user configuration files.
+
+Note that for efficiency the return value of the predicate function is
+cached the first time it is called.  Type \\[revert-buffer-quick] to
+invalidate the cached value."
+  :type '(choice
+          (const :tag "Don't activate" nil)
+          (const :tag "Always activate" t)
+          (const :tag "Activate for user files"
+                 elisp-flymake-byte-compile-user-file-p)
+          (function :tag "Predicate function"))
+  :group 'lisp
+  :version "31.1")
+
+(defvar-local elisp-flymake--byte-compile-activate-packages :unset
+  "Cached value for `elisp-flymake-byte-compile-activate-packages'.")
+
 ;;;###autoload
 (defun elisp-flymake-byte-compile (report-fn &rest _args)
   "A Flymake backend for elisp byte compilation.
@@ -2205,6 +2243,12 @@ elisp-flymake-byte-compile
     (save-restriction
       (widen)
       (write-region (point-min) (point-max) temp-file nil 'nomessage))
+    (when (eq elisp-flymake--byte-compile-activate-packages :unset)
+      (setq elisp-flymake--byte-compile-activate-packages
+            (if (functionp elisp-flymake-byte-compile-activate-packages)
+                (funcall elisp-flymake-byte-compile-activate-packages
+                         source-buffer)
+              elisp-flymake-byte-compile-activate-packages)))
     (let* ((output-buffer (generate-new-buffer " *elisp-flymake-byte-compile*"))
            ;; Hack: suppress warning about missing lexical cookie in
            ;; *scratch* buffers.
@@ -2223,6 +2267,8 @@ elisp-flymake-byte-compile
                    ;; "--eval" "(setq load-prefer-newer t)" ; for testing
                    ,@(mapcan (lambda (path) (list "-L" path))
                              elisp-flymake-byte-compile-load-path)
+                   ,@(when elisp-flymake--byte-compile-activate-packages
+                       '("-f" "package-activate-all"))
                    ,@warning-suppression-opt
                    "-f" "elisp-flymake--batch-compile-for-flymake"
                    ,temp-file)
-- 
2.47.0

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Pengji Zhang <me@pengjiz.com>
>> Cc: sbaugh@janestreet.com, João Távora
>>  <joaotavora@gmail.com>, Eli Zaretskii
>>  <eliz@gnu.org>
>> Date: Fri, 25 Oct 2024 19:50:48 +0800
>> 
>> João Távora <joaotavora@gmail.com> writes:
>> 
>> > Feel free to use my patch of course.  Stefan noted some time ago
>> > that -f package-initialize is the wrong flag to ask Emacs -Q to use
>> > elpa though, so there's something better.
>> 
>> Thanks! I prepared a new patch based on yours.
>
> Thanks.  Spencer, any comments?

I think this should also switch from passing "-Q" to passing just "-q".
If we're include ~/.emacs.d/elpa on load-path, we should include the
site-lisp directories too, which -Q suppresses.

Probably we should specifically pass "--batch --no-site-file" instead of
"-Q --batch", since --batch also implies -q.

(This is particularly relevant for my site, where most packages are
distributed via site-lisp/elpa rather than installed into
package-user-dir)

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Pengji Zhang]

Spencer Baugh <sbaugh@janestreet.com> writes:

> I think this should also switch from passing "-Q" to passing just
> "-q". If we're include ~/.emacs.d/elpa on load-path, we should include
> the site-lisp directories too, which -Q suppresses.

I agree. How about the following change?

--8<---------------cut here---------------start------------->8---
         :name "elisp-flymake-byte-compile"
         :buffer output-buffer
         :command `(,(expand-file-name invocation-name invocation-directory)
-                   "-Q"
+                   ,@(if elisp-flymake--byte-compile-activate-packages
+                         ;; Add site-lisp directories to `load-path' so
+                         ;; that system-wide packages (installed into
+                         ;; site-lisp/elpa) are activated.
+                         '("-q" "--no-site-file")
+                       '("-Q"))
                    "--batch"
                    ;; "--eval" "(setq load-prefer-newer t)" ; for testing
                    ,@(mapcan (lambda (path) (list "-L" path))
--8<---------------cut here---------------end--------------->8---

> Probably we should specifically pass "--batch --no-site-file" instead
> of "-Q --batch", since --batch also implies -q.

I am not sure if it is a good idea to always include the site-lisp
directories. I myself want this only when checking my init.el. When
developing packages, I prefer a pristine environment.

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Ship Mints]

[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]

Not sure this is 100% relevant to the discussion, but -Q also implies
--no-site-file which inhibits site-start.el. This will interfere with Emacs
builds such as https://github.com/jimeh/emacs-builds for macOS. They
rely on site-start to establish bundled native compiler runtime
dependencies. Running a flymake batch without those dependencies may have
unintended flymake performance implications.

On Thu, Nov 14, 2024 at 6:19 AM Pengji Zhang <me@pengjiz.com> wrote:

> Spencer Baugh <sbaugh@janestreet.com> writes:
>
> > I think this should also switch from passing "-Q" to passing just
> > "-q". If we're include ~/.emacs.d/elpa on load-path, we should include
> > the site-lisp directories too, which -Q suppresses.
>
> I agree. How about the following change?
>
> --8<---------------cut here---------------start------------->8---
>          :name "elisp-flymake-byte-compile"
>          :buffer output-buffer
>          :command `(,(expand-file-name invocation-name
> invocation-directory)
> -                   "-Q"
> +                   ,@(if elisp-flymake--byte-compile-activate-packages
> +                         ;; Add site-lisp directories to `load-path' so
> +                         ;; that system-wide packages (installed into
> +                         ;; site-lisp/elpa) are activated.
> +                         '("-q" "--no-site-file")
> +                       '("-Q"))
>                     "--batch"
>                     ;; "--eval" "(setq load-prefer-newer t)" ; for testing
>                     ,@(mapcan (lambda (path) (list "-L" path))
> --8<---------------cut here---------------end--------------->8---
>
> > Probably we should specifically pass "--batch --no-site-file" instead
> > of "-Q --batch", since --batch also implies -q.
>
> I am not sure if it is a good idea to always include the site-lisp
> directories. I myself want this only when checking my init.el. When
> developing packages, I prefer a pristine environment.
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 2833 bytes --]

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Eli Zaretskii]

Ping!  Can we make some progress with this bug report?

> Cc: joaotavora@gmail.com, 65035@debbugs.gnu.org
> From: Pengji Zhang <me@pengjiz.com>
> Date: Thu, 14 Nov 2024 19:18:20 +0800
> 
> Spencer Baugh <sbaugh@janestreet.com> writes:
> 
> > I think this should also switch from passing "-Q" to passing just
> > "-q". If we're include ~/.emacs.d/elpa on load-path, we should include
> > the site-lisp directories too, which -Q suppresses.
> 
> I agree. How about the following change?
> 
> --8<---------------cut here---------------start------------->8---
>          :name "elisp-flymake-byte-compile"
>          :buffer output-buffer
>          :command `(,(expand-file-name invocation-name invocation-directory)
> -                   "-Q"
> +                   ,@(if elisp-flymake--byte-compile-activate-packages
> +                         ;; Add site-lisp directories to `load-path' so
> +                         ;; that system-wide packages (installed into
> +                         ;; site-lisp/elpa) are activated.
> +                         '("-q" "--no-site-file")
> +                       '("-Q"))
>                     "--batch"
>                     ;; "--eval" "(setq load-prefer-newer t)" ; for testing
>                     ,@(mapcan (lambda (path) (list "-L" path))
> --8<---------------cut here---------------end--------------->8---
> 
> > Probably we should specifically pass "--batch --no-site-file" instead
> > of "-Q --batch", since --batch also implies -q.
> 
> I am not sure if it is a good idea to always include the site-lisp
> directories. I myself want this only when checking my init.el. When
> developing packages, I prefer a pristine environment.
> 
> 
> 
> 

bug#65035: 29.1; Port flycheck-emacs-lisp-initialize-packages to flymake

[Eli Zaretskii]

Ping! Ping!

> Cc: joaotavora@gmail.com, 65035@debbugs.gnu.org
> Date: Sat, 30 Nov 2024 11:48:11 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Ping!  Can we make some progress with this bug report?
> 
> > Cc: joaotavora@gmail.com, 65035@debbugs.gnu.org
> > From: Pengji Zhang <me@pengjiz.com>
> > Date: Thu, 14 Nov 2024 19:18:20 +0800
> > 
> > Spencer Baugh <sbaugh@janestreet.com> writes:
> > 
> > > I think this should also switch from passing "-Q" to passing just
> > > "-q". If we're include ~/.emacs.d/elpa on load-path, we should include
> > > the site-lisp directories too, which -Q suppresses.
> > 
> > I agree. How about the following change?
> > 
> > --8<---------------cut here---------------start------------->8---
> >          :name "elisp-flymake-byte-compile"
> >          :buffer output-buffer
> >          :command `(,(expand-file-name invocation-name invocation-directory)
> > -                   "-Q"
> > +                   ,@(if elisp-flymake--byte-compile-activate-packages
> > +                         ;; Add site-lisp directories to `load-path' so
> > +                         ;; that system-wide packages (installed into
> > +                         ;; site-lisp/elpa) are activated.
> > +                         '("-q" "--no-site-file")
> > +                       '("-Q"))
> >                     "--batch"
> >                     ;; "--eval" "(setq load-prefer-newer t)" ; for testing
> >                     ,@(mapcan (lambda (path) (list "-L" path))
> > --8<---------------cut here---------------end--------------->8---
> > 
> > > Probably we should specifically pass "--batch --no-site-file" instead
> > > of "-Q --batch", since --batch also implies -q.
> > 
> > I am not sure if it is a good idea to always include the site-lisp
> > directories. I myself want this only when checking my init.el. When
> > developing packages, I prefer a pristine environment.
> > 
> > 
> > 
> > 
> 
> 
> 
>