bug#33618: 27.0.50; ada-mode breaks M-x grep

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Stefan Monnier]

Package: Emacs
Version: 27.0.50


Opening a file in ada-mode using the new ada-mode from GNU ELPA globally
sets compilation-search-path (for me, it got set to `("~/tmp")`
probably because the Ada file was in ~/tmp), which in
turn breaks M-x grep in the sense that clicking on a match doesn't jump
to the file but prompts you to find the file (unless you happened to
grep from one of the directories mentioned in the
compilation-search-path, of course).


        Stefan



In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2018-11-24 built on pastel
Repository revision: 3f58f7c0289bf3fbd2e73c45561691b94cb1f6b1
Repository branch: work
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9 (stretch)

Recent messages:
previous-line: Beginning of buffer [2 times]
next-line: End of buffer [2 times]
Mark saved where search started
Mark set
Mark saved where search started
Auto-saving... [2 times]
Mark set
Undo!
Saving file /home/monnier/src/emacs/work/lisp/progmodes/grep.el...
Wrote /home/monnier/src/emacs/work/lisp/progmodes/grep.el

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Stefan Monnier]

> Opening a file in ada-mode using the new ada-mode from GNU ELPA globally
> sets compilation-search-path (for me, it got set to `("~/tmp")`
> probably because the Ada file was in ~/tmp), which in
> turn breaks M-x grep in the sense that clicking on a match doesn't jump
> to the file but prompts you to find the file (unless you happened to
> grep from one of the directories mentioned in the
> compilation-search-path, of course).

I use the patch below currently to work around this problem.
Should I install it into `master` (with a few more comments and
probably etc/NEWS or even manual updates)?


        Stefan


diff --git a/lisp/progmodes/ada-xref.el b/lisp/progmodes/ada-xref.el
index 359c187d85..d31e614f14 100644
--- a/lisp/progmodes/ada-xref.el
+++ b/lisp/progmodes/ada-xref.el
@@ -943,7 +943,7 @@ ada-select-prj-file
 	;; FIXME: use ada-get-absolute-dir, mapconcat here
 	(setenv "ADA_PROJECT_PATH" ada_project_path)))
 
-  (setq compilation-search-path (ada-xref-get-src-dir-field))
+  (setq-local compilation-search-path (ada-xref-get-src-dir-field))
 
   (setq ada-search-directories-internal
 	;; FIXME: why do we need directory-file-name here?
@@ -1208,9 +1208,8 @@ ada-compile-application
   (ada-require-project-file)
   (let ((cmd (ada-xref-get-project-field 'make_cmd))
 	(process-environment (ada-set-environment))
-	(compilation-scroll-output t))
-
-    (setq compilation-search-path (ada-xref-get-src-dir-field))
+	(compilation-scroll-output t)
+        (compilation-search-path (ada-xref-get-src-dir-field)))
 
     ;;  If no project file was found, ask the user
     (unless cmd
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index c7c510f7a3..f5627e10ce 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1614,7 +1614,7 @@ recompile
       (setq command (compilation-read-command (or (car compilation-arguments)
 						  command)))
       (if compilation-arguments (setcar compilation-arguments command)))
-    (apply 'compilation-start (or compilation-arguments (list command)))))
+    (apply #'compilation-start (or compilation-arguments (list command)))))
 
 (defcustom compilation-scroll-output nil
   "Non-nil to scroll the *compilation* buffer window as output appears.
@@ -1682,6 +1682,7 @@ compilation-start
 	    (replace-regexp-in-string "-mode\\'" "" (symbol-name mode))))
 	 (thisdir default-directory)
 	 (thisenv compilation-environment)
+         (this-search-path compilation-search-path)
 	 outwin outbuf)
     (with-current-buffer
 	(setq outbuf
@@ -1749,6 +1750,8 @@ compilation-start
         ;; NB: must be done after (funcall mode) as that resets local variables
         (set (make-local-variable 'compilation-directory) thisdir)
 	(set (make-local-variable 'compilation-environment) thisenv)
+        (unless (local-variable-p 'compilation-search-path)
+          (setq-local compilation-search-path this-search-path))
 	(if highlight-regexp
 	    (set (make-local-variable 'compilation-highlight-regexp)
 		 highlight-regexp))
diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 331eeec01b..16b54081ae 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -834,7 +834,8 @@ grep-mode
   (set (make-local-variable 'compilation-disable-input) t)
   (set (make-local-variable 'compilation-error-screen-columns)
        grep-error-screen-columns)
-  (add-hook 'compilation-filter-hook 'grep-filter nil t))
+  (setq-local compilation-search-path '(nil))
+  (add-hook 'compilation-filter-hook #'grep-filter nil t))
 
 (defun grep--save-buffers ()
   (when grep-save-buffers

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Tue, 04 Dec 2018 19:02:35 -0500
> Cc: Stephen Leake <stephen_leake@member.fsf.org>
> 
> > Opening a file in ada-mode using the new ada-mode from GNU ELPA globally
> > sets compilation-search-path (for me, it got set to `("~/tmp")`
> > probably because the Ada file was in ~/tmp), which in
> > turn breaks M-x grep in the sense that clicking on a match doesn't jump
> > to the file but prompts you to find the file (unless you happened to
> > grep from one of the directories mentioned in the
> > compilation-search-path, of course).
> 
> I use the patch below currently to work around this problem.
> Should I install it into `master` (with a few more comments and
> probably etc/NEWS or even manual updates)?

If the problem is in ada-mode in ELPA, it should be fixed there, IMO.
Why do we need to change our code to cater to problems in packages,
even if those packages are on ELPA?  It sounds wrong to me, FWIW.

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Stefan Monnier]

>> > Opening a file in ada-mode using the new ada-mode from GNU ELPA globally
>> > sets compilation-search-path (for me, it got set to `("~/tmp")`
>> > probably because the Ada file was in ~/tmp), which in
>> > turn breaks M-x grep in the sense that clicking on a match doesn't jump
>> > to the file but prompts you to find the file (unless you happened to
>> > grep from one of the directories mentioned in the
>> > compilation-search-path, of course).
>> 
>> I use the patch below currently to work around this problem.
>> Should I install it into `master` (with a few more comments and
>> probably etc/NEWS or even manual updates)?
>
> If the problem is in ada-mode in ELPA, it should be fixed there, IMO.
> Why do we need to change our code to cater to problems in packages,
> even if those packages are on ELPA?  It sounds wrong to me, FWIW.

Because I think the problem in ada-mode is linked to a design problem
with that variable: it is defined to be a global variable, and
compile.el looks it up from inside the compilation buffer, so there's no
convenient way for a major mode like ada-mode to tell compile.el which
search-path to use for which file/project: all they can do is change the
global value.

The patch I use changes compile.el so the var is looked up from the
buffer from which the compilation is launched (e.g. an ada-mode buffer)
and then stashed into the compilation buffer (for later use).

Note that the hunk below (which is part of the patch I sent) is
sufficient to unbreak grep, but other tools that (like grep) build on
`compilation-start` would still be affected.


        Stefan


--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -834,7 +834,8 @@ grep-mode
   (set (make-local-variable 'compilation-disable-input) t)
   (set (make-local-variable 'compilation-error-screen-columns)
        grep-error-screen-columns)
-  (add-hook 'compilation-filter-hook 'grep-filter nil t))
+  (setq-local compilation-search-path '(nil))
+  (add-hook 'compilation-filter-hook #'grep-filter nil t))
 
 (defun grep--save-buffers ()
   (when grep-save-buffers

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 33618@debbugs.gnu.org, stephen_leake@member.fsf.org
> Date: Wed, 05 Dec 2018 09:26:45 -0500
> 
> > Why do we need to change our code to cater to problems in packages,
> > even if those packages are on ELPA?  It sounds wrong to me, FWIW.
> 
> Because I think the problem in ada-mode is linked to a design problem
> with that variable: it is defined to be a global variable, and
> compile.el looks it up from inside the compilation buffer, so there's no
> convenient way for a major mode like ada-mode to tell compile.el which
> search-path to use for which file/project: all they can do is change the
> global value.

So how did we survive with this design problem until now?

> The patch I use changes compile.el so the var is looked up from the
> buffer from which the compilation is launched (e.g. an ada-mode buffer)
> and then stashed into the compilation buffer (for later use).

What will that do if I invoke, e.g., "M-x recompile" from a source
buffer other than the one from which I invoked the previous "M-x compile"?
And what if we have multiple compilation buffers?

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Eli Zaretskii]

> Date: Wed, 05 Dec 2018 16:37:28 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: stephen_leake@member.fsf.org, 33618@debbugs.gnu.org
> 
> > The patch I use changes compile.el so the var is looked up from the
> > buffer from which the compilation is launched (e.g. an ada-mode buffer)
> > and then stashed into the compilation buffer (for later use).
> 
> What will that do if I invoke, e.g., "M-x recompile" from a source
> buffer other than the one from which I invoked the previous "M-x compile"?
> And what if we have multiple compilation buffers?

And, btw, isn't it wrong for a mode to set the value of a defcustom?

Maybe we should have a separate variable for this purpose, one that
isn't a defcustom.  A buffer-local value of a defcustom is going to
surprise users, I think.

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Stefan Monnier]

>> Because I think the problem in ada-mode is linked to a design problem
>> with that variable: it is defined to be a global variable, and
>> compile.el looks it up from inside the compilation buffer, so there's no
>> convenient way for a major mode like ada-mode to tell compile.el which
>> search-path to use for which file/project: all they can do is change the
>> global value.
>
> So how did we survive with this design problem until now?

I don't know.  I think by and large no package/user used it (After all,
in most cases compiler messages include the absolute file name IME), or
they used it only in Emacs sessions that are used for a single project.

>> The patch I use changes compile.el so the var is looked up from the
>> buffer from which the compilation is launched (e.g. an ada-mode buffer)
>> and then stashed into the compilation buffer (for later use).
> What will that do if I invoke, e.g., "M-x recompile" from a source
> buffer other than the one from which I invoked the previous "M-x compile"?

Good question.  I guess M-x recompile should first switch to the
compilation buffer and then cause the recompile from there (hence
reusing the value that was stashed into the compilation buffer).
[ Note: I haven't checked to see if my patch does that or not.  ]

> And what if we have multiple compilation buffers?

Not sure why that would make a difference.

> And, btw, isn't it wrong for a mode to set the value of a defcustom?

Yes.

> Maybe we should have a separate variable for this purpose, one that
> isn't a defcustom.  A buffer-local value of a defcustom is going to
> surprise users, I think.

Sounds like a good idea, tho we'd have to figure out how to combine the
two variable's values.


        Stefan

bug#33618: emacs ada-mode bug 33618

[Stephen Leake]

I did not see this bug until now; apparently my fsf email forwarding is
not working (I'm looking into that).

Stefan writes:

> Opening a file in ada-mode using the new ada-mode from GNU ELPA globally
> sets compilation-search-path (for me, it got set to `("~/tmp")`
> probably because the Ada file was in ~/tmp), which in
> turn breaks M-x grep in the sense that clicking on a match doesn't jump
> to the file but prompts you to find the file (unless you happened to
> grep from one of the directories mentioned in the
> compilation-search-path, of course).

Yes; 'compilation-search-path' is an attribute of a "project" (a very
poorly defined concept in Emacs, unfortunately). Opening an ada-mode
file defines a default project, which has the current directory as the
value for 'compilation-search-path'.

> Because I think the problem in ada-mode is linked to a design problem
> with that variable: it is defined to be a global variable, and
> compile.el looks it up from inside the compilation buffer, so there's no
> convenient way for a major mode like ada-mode to tell compile.el which
> search-path to use for which file/project: all they can do is change the
> global value.

Yes.

> The patch I use changes compile.el so the var is looked up from the
> buffer from which the compilation is launched (e.g. an ada-mode buffer)
> and then stashed into the compilation buffer (for later use).

This is one approach. I'll install it in my working emacs and see if it
causes me any problems. I suspect it will break my project code, which
assumes the global value of 'compilation-search-path' is used.

Another approach would be to use the 'project' package; if
(current-project) returns non-nil, use (project-path (current-project)).
But there is no 'project-path' (despite my efforts to define one), so
that doesn't work. There is only 'project-roots' and
'project-library-roots', which return values not suitable for
'compilation-search-path'. Perhaps this bug provides a context to reopen
that discussion.

ada-mode does provide an explicit notion of projects, which include the
list of source directories, which is used to set
compilation-search-path. When the user has specified such a project, it
would be surprising to see some other value of compilation-search-path
be used, based on what buffer happens to be current; I view the choice
of 'project' as global, affecting all files/buffers, because a
real-world project involves many kinds of files, not just Ada ones.

It would make sense to factor out the project-related code in ada-mode,
and turn it into a project-<something> package.

The original problem in this bug was caused by ada-mode providing a
default project when you had not specified one. The goal there is to
make it easy for Emacs/ada-mode newbies to invoke the various
ada-related tools (mainly the compiler), before learning about ada-mode
projects. Perhaps that design choice should be reconsidered.

Eli writes:

> And, btw, isn't it wrong for a mode to set the value of a defcustom?

Good point.

> Maybe we should have a separate variable for this purpose, one that
> isn't a defcustom.  A buffer-local value of a defcustom is going to
> surprise users, I think.

Yes.

(project-source-path (project-current)) seems to be the right choice
here, if we can define that in a reasonable way.

I didn't use that in ada-mode before, because I was trying to maintain
compatibility with Emacs 24. Now that I've given up on that, ada-mode
can integrate with project.el

Stefan writes:

> I think by and large no package/user used it (After all,
> in most cases compiler messages include the absolute file name IME),

The GNAT Ada compiler options can be set to provide the full path, but I
prefer not to waste that screen space, since Emacs is perfectly capable
of finding the right file.

> or they used it only in Emacs sessions that are used for a single
> project.

Yes, there seems to be a strong preference (bias?) towards only allowing
a single project per Emacs process. Which means you have to exit Emacs
to change projects. I've never liked that behaviour in other IDEs, and
I've been using multiple projects in Emacs for years.

-- 
-- Stephe

bug#33618: emacs ada-mode bug 33618

[Dmitry Gutov]

On 20.12.2018 22:17, Stephen Leake wrote:
> Perhaps this bug provides a context to reopen
> that discussion.

Perhaps.

> (project-source-path (project-current)) seems to be the right choice
> here, if we can define that in a reasonable way.

 From what I'm seeing of this variable, it doesn't like like a 
fundamental property of the project to me personally (*), so I still 
think it was the right choice to omit it back then, or at least not base 
the file-enumeration logic on it.

(*) None of the environments I work in show error location relative to 
some path entries, as opposed to providing either absolute file name, or 
file names relative to the current directory.

So I'd like to see more voices from people who do work in such environments.

> Yes, there seems to be a strong preference (bias?) towards only allowing
> a single project per Emacs process.

I'm not sure we should encourage it either. I do know that some users, 
at least, open several projects in one Emacs session at the same time.

bug#33618: update

[Stephen Leake]

I did not see this bug until now; apparently my fsf email forwarding is
not working (I'm looking into that).

Stefan writes:

> Opening a file in ada-mode using the new ada-mode from GNU ELPA globally
> sets compilation-search-path (for me, it got set to `("~/tmp")`
> probably because the Ada file was in ~/tmp), which in
> turn breaks M-x grep in the sense that clicking on a match doesn't jump
> to the file but prompts you to find the file (unless you happened to
> grep from one of the directories mentioned in the
> compilation-search-path, of course).

Yes; 'compilation-search-path' is an attribute of a "project" (a very
poorly defined concept in Emacs, unfortunately). Opening an ada-mode
file defines a default project, which has the current directory as the
value for 'compilation-search-path'.

> Because I think the problem in ada-mode is linked to a design problem
> with that variable: it is defined to be a global variable, and
> compile.el looks it up from inside the compilation buffer, so there's no
> convenient way for a major mode like ada-mode to tell compile.el which
> search-path to use for which file/project: all they can do is change the
> global value.

Yes.

> The patch I use changes compile.el so the var is looked up from the
> buffer from which the compilation is launched (e.g. an ada-mode buffer)
> and then stashed into the compilation buffer (for later use).

This is one approach. I'll install it in my working emacs and see if it
causes me any problems. I suspect it will break my project code, which
assumes the global value of 'compilation-search-path' is used.

Another approach would be to use the 'project' package; if
(current-project) returns non-nil, use (project-path (current-project)).
But there is no 'project-path' (despite my efforts to define one), so
that doesn't work. There is only 'project-roots' and
'project-library-roots', which return values not suitable for
'compilation-search-path'. Perhaps this bug provides a context to reopen
that discussion.

ada-mode does provide an explicit notion of projects, which include the
list of source directories, which is used to set
compilation-search-path. When the user has specified such a project, it
would be surprising to see some other value of compilation-search-path
be used, based on what buffer happens to be current; I view the choice
of 'project' as global, affecting all files/buffers, because a
real-world project involves many kinds of files, not just Ada ones.

It would make sense to factor out the project-related code in ada-mode,
and turn it into a project-<something> package.

The original problem in this bug was caused by ada-mode providing a
default project when you had not specified one. The goal there is to
make it easy for Emacs/ada-mode newbies to invoke the various
ada-related tools (mainly the compiler), before learning about ada-mode
projects. Perhaps that design choice should be reconsidered.

Eli writes:

> And, btw, isn't it wrong for a mode to set the value of a defcustom?

Good point.

> Maybe we should have a separate variable for this purpose, one that
> isn't a defcustom.  A buffer-local value of a defcustom is going to
> surprise users, I think.

Yes.

(project-source-path (project-current)) seems to be the right choice
here, if we can define that in a reasonable way.

I didn't use that in ada-mode before, because I was trying to maintain
compatibility with Emacs 24. Now that I've given up on that, ada-mode
can integrate with project.el

Stefan writes:

> I think by and large no package/user used it (After all,
> in most cases compiler messages include the absolute file name IME),

The GNAT Ada compiler options can be set to provide the full path, but I
prefer not to waste that screen space, since Emacs is perfectly capable
of finding the right file.

> or they used it only in Emacs sessions that are used for a single
> project.

Yes, there seems to be a strong preference (bias?) towards only allowing
a single project per Emacs process. Which means you have to exit Emacs
to change projects. I've never liked that behaviour in other IDEs, and
I've been using multiple projects in Emacs for years.

-- 
-- Stephe

bug#33618: 27.0.50; ada-mode breaks M-x grep

[Ludovic Brenta]

Hello,

I'd like to chime in as one of a team of 25 people using emacs ada-mode
on a large software project for the past 20+ years.  We usually use one
"project" per emacs process only, so we're not concerned about 
supporting
multiple "projects" in an emacs process.  However we are very concerned
about compilation buffers containing absolute paths.  Our source files 
are
spread in about 50 directories, the full paths of which are routinely
longer than 120 characters.  Inside each directory are hundreds of 
source
files, the names of which can be another 40 characters each.  We use 
etags
and gnatfind to navigate quickly and efficiently between these files,
without ever needing to remember, type or see the full directory name.
This is especially true in a compilation buffer; RET simply jumps to the
correct file even if only its name is displayed.

So, we've also been hit by this bug when running grep but quickly worked
around it in our ~/.emacs like so:

(add-hook 'grep-mode-hook
           (lambda ()
             "In grep buffers, do not use the compilation-search-path to 
look for source files.  Make a local binding
to the variable that overrides the one from any Ada mode or other 
language project files."
             (set (make-local-variable 'compilation-search-path) 
'(nil))))

and we left it at that until now.

I do agree that the idea of a single, global, defcustom
compilation-search-path seems to make too many assumptions about the
various tools that can run in a compilation buffer, their current 
working
directory, which source file (if any) they were called from, etc.. so
maybe this idea should be revisited.  I do like the ideas so far:

- make a new, buffer-local, non-defcustom variable;
- copy its value into a buffer-local variable when creating a new
   compilation buffer;
- reuse the buffer-local variable of a compilation buffer when reusing 
the
   buffer (i.e. recompile).

-- 
Ludovic Brenta.

bug#33618: better fix

[Stephen Leake]

`grep-mode' is defined by `define-compilation-mode', which sets
`compilation-search-path' to a buffer-local copy of `grep-search-path',
if the latter is defined.

So there is precedent for making custom vars buffer-local, and it seems
the proper fix for this bug is to define `grep-search-path':

--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -441,6 +441,14 @@ grep-find-abbreviate
   :version "27.1"
   :group 'grep)
 
+(defcustom grep-search-path '(nil)
+  "Search path for grep results.
+Elements should be directory names, not file names of directories.
+The value nil as an element means to try the default directory."
+  :group 'grep
+  :type '(repeat (choice (const :tag "Default" nil)
+			 (string :tag "Directory"))))
+
 (defvar grep-find-abbreviate-properties
   (let ((ellipsis (if (char-displayable-p ?…) "[…]" "[...]"))
         (map (make-sparse-keymap)))


`emacs-lisp-compilation-mode' needs a similar fix:

--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1022,6 +1022,14 @@ emacs-lisp-compilation-parse-errors-filename-function
   "The value for `compilation-parse-errors-filename-function' for when
 we go into emacs-lisp-compilation-mode.")
 
+(defcustom emacs-lisp-compilation-search-path '(nil)
+  "Search path for byte-compile error messages.
+Elements should be directory names, not file names of directories.
+The value nil as an element means to try the default directory."
+  :group 'bytecomp
+  :type '(repeat (choice (const :tag "Default" nil)
+			 (string :tag "Directory"))))
+
 (define-compilation-mode emacs-lisp-compilation-mode "elisp-compile"
   "The variant of `compilation-mode' used for emacs-lisp error buffers")

-- 
-- Stephe

bug#33618: better fix

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Date: Sat, 20 Apr 2019 08:47:47 -0800
> 
> `grep-mode' is defined by `define-compilation-mode', which sets
> `compilation-search-path' to a buffer-local copy of `grep-search-path',
> if the latter is defined.
> 
> So there is precedent for making custom vars buffer-local, and it seems
> the proper fix for this bug is to define `grep-search-path':

Please don't forget :version tags of new defcustoms, and also they
should be called out in NEWS.

Thanks.

bug#33618: fixed in commit 1486eadf7c9469f873fcd04beafd03f21564d580

[Stephen Leake]

-- 
-- Stephe