GNU bug report logs - #24585
25.1; avoid hack in ggtags.el to run compilation-auto-jump timer

Previous Next

Package: emacs;

Reported by: Leo Liu <sdl.web <at> gmail.com>

Date: Sun, 2 Oct 2016 04:58:02 UTC

Severity: normal

Tags: moreinfo

Found in version 25.1

Done: Lars Ingebrigtsen <larsi <at> gnus.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24585 in the body.
You can then email your comments to 24585 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Sun, 02 Oct 2016 04:58:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Leo Liu <sdl.web <at> gmail.com>:
New bug report received and forwarded. Copy sent to monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org. (Sun, 02 Oct 2016 04:58:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Sun, 02 Oct 2016 12:56:40 +0800
,----[ Stefan Monnier ]
| > +      ;; Manually run the `compilation-auto-jump' timer. Hackish but
| > +      ;; everything else seems unreliable. See:
| > +      ;;
| > +      ;; - http://debbugs.gnu.org/13829
| > +      ;; - http://debbugs.gnu.org/23987
| > +      ;; - https://github.com/leoliu/ggtags/issues/89
| > +      ;;
| > +      (pcase (cl-find 'compilation-auto-jump timer-list :key #'timer--function)
| > +        (`nil )
| > +        (timer (timer-event-handler timer)))
| 
| I think this deserves a bug report, where we try to figure out how to
| make it possible for ggtags.el to work properly without resorting to such
| a hideous and brittle hack.
| 
| A hack will probably have to stay for compatibility with older Emacsen,
| but we should come up with a better solution for the future.
`----

The hack is due to difficulty in making sure one timer is run before
another that run nearly at the same time. And it's made harder because
timer.el prepends a timer to existing timers running at the same time,
which looks like a mistake.

I think the hack in ggtags.el may be removed by the following patch to
correct the said mistake:

diff --git a/lisp/emacs-lisp/timer.el b/lisp/emacs-lisp/timer.el
index c01ea497..337e1049 100644
--- a/lisp/emacs-lisp/timer.el
+++ b/lisp/emacs-lisp/timer.el
@@ -130,9 +130,9 @@ floating point number."
 	(setq delta (time-add delta (list 0 0 (or usecs 0) (or psecs 0)))))
     (time-add time delta)))
 
-(defun timer--time-less-p (t1 t2)
+(defun timer--time-less-or-equal-p (t1 t2)
   "Say whether time value T1 is less than time value T2."
-  (time-less-p (timer--time t1) (timer--time t2)))
+  (not (time-less-p (timer--time t2) (timer--time t1))))
 
 (defun timer-inc-time (timer secs &optional usecs psecs)
   "Increment the time set in TIMER by SECS seconds, USECS microseconds,
@@ -172,7 +172,7 @@ fire repeatedly that many seconds apart."
       (let ((timers (if idle timer-idle-list timer-list))
 	    last)
 	;; Skip all timers to trigger before the new one.
-	(while (and timers (timer--time-less-p (car timers) timer))
+	(while (and timers (timer--time-less-or-equal-p (car timers) timer))
 	  (setq last timers
 		timers (cdr timers)))
 	(if reuse-cell




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Sun, 02 Oct 2016 20:37:01 GMT) Full text and rfc822 format available.

Message #8 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Sun, 02 Oct 2016 16:35:57 -0400
> I think the hack in ggtags.el may be removed by the following patch to
> correct the said mistake:

IIUC this patch just changes the ordering for same-time timers.
Relying on either ordering is itself a hack.  We need a better solution.

What are those two timers whose relative execution order matters?
Why do they care in which order they're run?


        Stefan


> diff --git a/lisp/emacs-lisp/timer.el b/lisp/emacs-lisp/timer.el
> index c01ea497..337e1049 100644
> --- a/lisp/emacs-lisp/timer.el
> +++ b/lisp/emacs-lisp/timer.el
> @@ -130,9 +130,9 @@ floating point number."
>  	(setq delta (time-add delta (list 0 0 (or usecs 0) (or psecs 0)))))
>      (time-add time delta)))
 
> -(defun timer--time-less-p (t1 t2)
> +(defun timer--time-less-or-equal-p (t1 t2)
>    "Say whether time value T1 is less than time value T2."
> -  (time-less-p (timer--time t1) (timer--time t2)))
> +  (not (time-less-p (timer--time t2) (timer--time t1))))
 
>  (defun timer-inc-time (timer secs &optional usecs psecs)
>    "Increment the time set in TIMER by SECS seconds, USECS microseconds,
> @@ -172,7 +172,7 @@ fire repeatedly that many seconds apart."
>        (let ((timers (if idle timer-idle-list timer-list))
>  	    last)
>  	;; Skip all timers to trigger before the new one.
> -	(while (and timers (timer--time-less-p (car timers) timer))
> +	(while (and timers (timer--time-less-or-equal-p (car timers) timer))
>  	  (setq last timers
>  		timers (cdr timers)))
>  	(if reuse-cell






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Mon, 03 Oct 2016 03:02:02 GMT) Full text and rfc822 format available.

Message #11 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Mon, 03 Oct 2016 11:01:02 +0800
On 2016-10-02 16:35 -0400, Stefan Monnier wrote:
> IIUC this patch just changes the ordering for same-time timers.
> Relying on either ordering is itself a hack.  We need a better solution.

If you eval (progn (run-with-timer 0 ...)  ; timer 1
                   (run-with-timer 0 ...)) ; timer 2

you expect timer 1 to be triggered first, no?

> What are those two timers whose relative execution order matters?
> Why do they care in which order they're run?

The first timer is compilation-auto-jump which is installed (by compile)
at the start of compilation.

The second timer is a cleanup timer which is installed (by ggtags) when
compilation finishes and there is 0 or 1 match.

The second timer kills the buffer (among other things) that the first
timer depends on.

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Mon, 03 Oct 2016 13:25:02 GMT) Full text and rfc822 format available.

Message #14 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Mon, 03 Oct 2016 09:24:50 -0400
>> IIUC this patch just changes the ordering for same-time timers.
>> Relying on either ordering is itself a hack.  We need a better solution.
> If you eval (progn (run-with-timer 0 ...)  ; timer 1
>                    (run-with-timer 0 ...)) ; timer 2
> you expect timer 1 to be triggered first, no?

Not really.  I think any code which relies on such a property will
sooner suffer.

>> What are those two timers whose relative execution order matters?
>> Why do they care in which order they're run?
> The first timer is compilation-auto-jump which is installed (by compile)
> at the start of compilation.
> The second timer is a cleanup timer which is installed (by ggtags) when
> compilation finishes and there is 0 or 1 match.
> The second timer kills the buffer (among other things) that the first
> timer depends on.

So we can fix the bug by making the timer's code check if the buffer is
still live?


        Stefan




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Mon, 03 Oct 2016 15:23:02 GMT) Full text and rfc822 format available.

Message #17 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Mon, 03 Oct 2016 23:22:24 +0800
On 2016-10-03 09:24 -0400, Stefan Monnier wrote:
>> The first timer is compilation-auto-jump which is installed (by compile)
>> at the start of compilation.
>> The second timer is a cleanup timer which is installed (by ggtags) when
>> compilation finishes and there is 0 or 1 match.
>> The second timer kills the buffer (among other things) that the first
>> timer depends on.
>
> So we can fix the bug by making the timer's code check if the buffer is
> still live?

No. We still want compilation-auto-jump to happen first. Otherwise we
just find 1 match and clean up without jumping to the match.

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Tue, 04 Oct 2016 11:48:01 GMT) Full text and rfc822 format available.

Message #20 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Mon, 03 Oct 2016 14:45:57 -0400
>>> The first timer is compilation-auto-jump which is installed (by compile)
>>> at the start of compilation.
>>> The second timer is a cleanup timer which is installed (by ggtags) when
>>> compilation finishes and there is 0 or 1 match.
>>> The second timer kills the buffer (among other things) that the first
>>> timer depends on.
>> So we can fix the bug by making the timer's code check if the buffer is
>> still live?
> No.  We still want compilation-auto-jump to happen first.  Otherwise we
> just find 1 match and clean up without jumping to the match.

Then why does the second timer kill the buffer so hastily?


        Stefan




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Tue, 04 Oct 2016 13:09:01 GMT) Full text and rfc822 format available.

Message #23 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Tue, 04 Oct 2016 21:08:07 +0800
On 2016-10-03 14:45 -0400, Stefan Monnier wrote:
> Then why does the second timer kill the buffer so hastily?
>
>
>         Stefan

ggtags-navigation-mode-cleanup unhides the window popped up by
compilation-auto-jump. If we postpone it we see the window configuration
on the frame messed up, which is kinda annoying.

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Tue, 04 Oct 2016 16:19:02 GMT) Full text and rfc822 format available.

Message #26 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Tue, 04 Oct 2016 12:18:22 -0400
>> Then why does the second timer kill the buffer so hastily?
> ggtags-navigation-mode-cleanup unhides the window popped up by
> compilation-auto-jump. If we postpone it we see the window configuration
> on the frame messed up, which is kinda annoying.

That would explain why you want to remove the buffer from specific
windows (i.e. hide the buffer), but not why you need to kill it.


        Stefan




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Wed, 05 Oct 2016 07:40:02 GMT) Full text and rfc822 format available.

Message #29 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Wed, 05 Oct 2016 15:39:02 +0800
On 2016-10-04 12:18 -0400, Stefan Monnier wrote:
> That would explain why you want to remove the buffer from specific
> windows (i.e. hide the buffer), but not why you need to kill it.

The buffer is not needed by ggtags so kill was chosen. I could change it
to not kill but this won't fix the crux of the issue i.e.

  If compilation-auto-jump is run after cleanup it will mess up things
  for example pop up windows.

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Wed, 05 Oct 2016 10:27:02 GMT) Full text and rfc822 format available.

Message #32 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Wed, 05 Oct 2016 13:25:45 +0300
> From: Leo Liu <sdl.web <at> gmail.com>
> Date: Wed, 05 Oct 2016 15:39:02 +0800
> Cc: 24585 <at> debbugs.gnu.org
> 
> On 2016-10-04 12:18 -0400, Stefan Monnier wrote:
> > That would explain why you want to remove the buffer from specific
> > windows (i.e. hide the buffer), but not why you need to kill it.
> 
> The buffer is not needed by ggtags so kill was chosen. I could change it
> to not kill but this won't fix the crux of the issue i.e.
> 
>   If compilation-auto-jump is run after cleanup it will mess up things
>   for example pop up windows.

Instead of adding another timer, and then dealing with their order
issues, is it possible to reuse the same time for both jobs?  IOW,
just _add_ the new/additional function you need to be done to whatever
the single timer does.  If you do that, you get to control the order
in which things are done by the single timer, because you write the
code of the timer function(s).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Thu, 06 Oct 2016 16:14:02 GMT) Full text and rfc822 format available.

Message #35 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 00:12:57 +0800
On 2016-10-05 13:25 +0300, Eli Zaretskii wrote:
> Instead of adding another timer, and then dealing with their order
> issues, is it possible to reuse the same time for both jobs?  IOW,
> just _add_ the new/additional function you need to be done to whatever
> the single timer does.  If you do that, you get to control the order
> in which things are done by the single timer, because you write the
> code of the timer function(s).

probably not.

ggtags-global-handle-exit needs to consider these situations:

1. when matches are large jump to first match should not wait until
   compilation finishes

2. on failure keep the compilation window popped-up so users can see the
   errors

3. 0 matches, nothing to show so clean up

4. 1 match, wait until compilation-auto-jump and clean up

A solution has to cover all of them. I think this could work:

1. In compilation-error-properties save the jump pos like this

  (setq-local compilation-auto-jump-pos (match-beginning 0))
  (run-with-timer 0 nil 'compilation-auto-jump
                        (current-buffer) compilation-auto-jump-pos)

2. Make compilation-auto-jump do nothing if the buffer is killed.

WDYT?

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Thu, 06 Oct 2016 18:11:02 GMT) Full text and rfc822 format available.

Message #38 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Thu, 06 Oct 2016 14:10:40 -0400
> 2. Make compilation-auto-jump do nothing if the buffer is killed.
> WDYT?

I thought that's what I had proposed, so yes, I think that's
a good solution.


        Stefan




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Thu, 06 Oct 2016 18:32:02 GMT) Full text and rfc822 format available.

Message #41 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 24585 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 02:31:39 +0800
On 2016-10-06 14:10 -0400, Stefan Monnier wrote:
> I thought that's what I had proposed, so yes, I think that's
> a good solution.

By exposing the position I can change ggtags not to depend on the order
of auto-jump and clean up.

The patch could look like this. Any objection to installing it in
emacs-25?


diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index f2e397a4..a7804fd7 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1052,13 +1052,16 @@ POS and RES.")
 	      l2
 	    (setcdr l1 (cons (list ,key) l2)))))))
 
+(defvar-local compilation-auto-jump-pos nil)
+
 (defun compilation-auto-jump (buffer pos)
-  (with-current-buffer buffer
-    (goto-char pos)
-    (let ((win (get-buffer-window buffer 0)))
-      (if win (set-window-point win pos)))
-    (if compilation-auto-jump-to-first-error
-	(compile-goto-error))))
+  (when (buffer-live-p buffer)
+    (with-current-buffer buffer
+      (goto-char pos)
+      (let ((win (get-buffer-window buffer 0)))
+        (if win (set-window-point win pos)))
+      (if compilation-auto-jump-to-first-error
+          (compile-goto-error)))))
 
 ;; This function is the central driver, called when font-locking to gather
 ;; all information needed to later jump to corresponding source code.
@@ -1126,8 +1129,9 @@ POS and RES.")
     (when (and compilation-auto-jump-to-next
                (>= type compilation-skip-threshold))
       (kill-local-variable 'compilation-auto-jump-to-next)
+      (setq compilation-auto-jump-pos (match-beginning 0))
       (run-with-timer 0 nil 'compilation-auto-jump
-                      (current-buffer) (match-beginning 0)))
+                      (current-buffer) compilation-auto-jump-pos))
 
     (compilation-internal-error-properties
      file line end-line col end-col type fmt)))




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Thu, 06 Oct 2016 18:38:01 GMT) Full text and rfc822 format available.

Message #44 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Thu, 06 Oct 2016 21:37:01 +0300
> From:  Leo Liu <sdl.web <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 24585 <at> debbugs.gnu.org
> Date: Fri, 07 Oct 2016 02:31:39 +0800
> 
> The patch could look like this. Any objection to installing it in
> emacs-25?

That depends on how long is this problem present.  Can you tell?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 01:22:02 GMT) Full text and rfc822 format available.

Message #47 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 09:21:12 +0800
On 2016-10-06 21:37 +0300, Eli Zaretskii wrote:
> That depends on how long is this problem present.  Can you tell?

Originally I thought by inserting a (sit-for 0) I fixed the problem but
people have reported otherwise
https://github.com/leoliu/ggtags/issues/89 (which is 20 months old)

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 02:29:02 GMT) Full text and rfc822 format available.

Message #50 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 10:27:49 +0800
On 2016-10-07 09:21 +0800, Leo Liu wrote:
> On 2016-10-06 21:37 +0300, Eli Zaretskii wrote:
>> That depends on how long is this problem present.  Can you tell?
>
> Originally I thought by inserting a (sit-for 0) I fixed the problem but
> people have reported otherwise
> https://github.com/leoliu/ggtags/issues/89 (which is 20 months old)
>
> Leo

After some experiments the proposed patch might not work at all.

compilation-auto-jump timer is fired by the font-locking engine. By my
observation it is possible that compilation-finish-functions (from a
process sentinel) are executed before font-locking even by inserting

  (compilation--ensure-parse (point-max))

in ggtags-global-handle-exit.

Do you see any anomaly in the observation?

The hack that is now in ggtags.el may not work either. None of the
people that reported https://github.com/leoliu/ggtags/issues/89 have
confirmed the fix. And I personally don't know how to reproduce it.

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 08:01:01 GMT) Full text and rfc822 format available.

Message #53 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 10:59:52 +0300
> From:  Leo Liu <sdl.web <at> gmail.com>
> Cc: 24585 <at> debbugs.gnu.org,  monnier <at> IRO.UMontreal.CA
> Date: Fri, 07 Oct 2016 09:21:12 +0800
> 
> On 2016-10-06 21:37 +0300, Eli Zaretskii wrote:
> > That depends on how long is this problem present.  Can you tell?
> 
> Originally I thought by inserting a (sit-for 0) I fixed the problem but
> people have reported otherwise
> https://github.com/leoliu/ggtags/issues/89 (which is 20 months old)

Thanks.  That's when the problem was reported, not when it was
introduced.  However, it does mean the problem existed even before Feb
2015, which means it existed in Emacs 24.5.

OTOH, the fix seems quite safe.  So yes, please push to emacs-25.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 08:04:01 GMT) Full text and rfc822 format available.

Message #56 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, monnier <at> IRO.UMontreal.CA
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 11:02:48 +0300
> From:  Leo Liu <sdl.web <at> gmail.com>
> Cc: 24585 <at> debbugs.gnu.org,  monnier <at> IRO.UMontreal.CA
> Date: Fri, 07 Oct 2016 10:27:49 +0800
> 
> compilation-auto-jump timer is fired by the font-locking engine. By my
> observation it is possible that compilation-finish-functions (from a
> process sentinel) are executed before font-locking even by inserting
> 
>   (compilation--ensure-parse (point-max))
> 
> in ggtags-global-handle-exit.
> 
> Do you see any anomaly in the observation?

Not in these general terms, no.

> The hack that is now in ggtags.el may not work either. None of the
> people that reported https://github.com/leoliu/ggtags/issues/89 have
> confirmed the fix. And I personally don't know how to reproduce it.

So what is your proposal?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 12:47:02 GMT) Full text and rfc822 format available.

Message #59 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 08:46:10 -0400
> compilation-auto-jump timer is fired by the font-locking engine.

Not sure what you mean by "fired", but in any case, no: font-lock is not
used for that (tho it has been used at some point before
compilation-auto-jump was introduced, IIRC).  Instead, it's done via
syntax-propertize (which can be triggered in all kinds of ways,
including font-lock).

How 'bout something like the following:

- Add a new var compilation-pending-auto-jump set buffer-locally to
  non-nil when compilation-error-properties calls run-with-timer.
- in compilation-auto-jump, check this var before doing anything and set
  it back to nil.
- in ggtags, call compilation-auto-jump to make sure this timer is run
  before yours.


        Stefan




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 17:08:01 GMT) Full text and rfc822 format available.

Message #62 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Sat, 08 Oct 2016 01:07:30 +0800
On 2016-10-07 08:46 -0400, Stefan Monnier wrote:
> Not sure what you mean by "fired", but in any case, no: font-lock is not
> used for that (tho it has been used at some point before
> compilation-auto-jump was introduced, IIRC).  Instead, it's done via
> syntax-propertize (which can be triggered in all kinds of ways,
> including font-lock).

This

(defun compilation-mode-font-lock-keywords ()
  "Return expressions to highlight in Compilation mode."
  (append
   '((compilation--ensure-parse))
   compilation-mode-font-lock-keywords))

and the only place in compile.el that mentions syntax-propertize is in a
comment.

> How 'bout something like the following:
>
> - Add a new var compilation-pending-auto-jump set buffer-locally to
>   non-nil when compilation-error-properties calls run-with-timer.
> - in compilation-auto-jump, check this var before doing anything and set
>   it back to nil.
> - in ggtags, call compilation-auto-jump to make sure this timer is run
>   before yours.

I think the issue is compilation-error-properties can happen after
compilation-finish-functions. And calling compilation--ensure-parse in
ggtags-global-handle-exit doesn't seem to help.

Leo




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 07 Oct 2016 18:11:02 GMT) Full text and rfc822 format available.

Message #65 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Fri, 07 Oct 2016 14:10:53 -0400
>> Not sure what you mean by "fired", but in any case, no: font-lock is not
>> used for that (tho it has been used at some point before
>> compilation-auto-jump was introduced, IIRC).  Instead, it's done via
>> syntax-propertize (which can be triggered in all kinds of ways,
>> including font-lock).
>
> This
>
> (defun compilation-mode-font-lock-keywords ()
>   "Return expressions to highlight in Compilation mode."
>   (append
>    '((compilation--ensure-parse))
>    compilation-mode-font-lock-keywords))
>
> and the only place in compile.el that mentions syntax-propertize is in a
> comment.

Duh!  Indeed, sorry, I mis-remembered.  Indeed, after first (ab)using
syntax-propertize I changed it to use the same approach but without
using syntax-propertize.

The point is the same, tho: the parse can be triggered by other things
than font-lock.  That's important because font-lock may be turned off.

>> How 'bout something like the following:
>>
>> - Add a new var compilation-pending-auto-jump set buffer-locally to
>> non-nil when compilation-error-properties calls run-with-timer.
>> - in compilation-auto-jump, check this var before doing anything and set
>> it back to nil.
>> - in ggtags, call compilation-auto-jump to make sure this timer is run
>> before yours.
> I think the issue is compilation-error-properties can happen after
> compilation-finish-functions. And calling compilation--ensure-parse in
> ggtags-global-handle-exit doesn't seem to help.

Hmm... calling compilation--ensure-parse *should* help.
More specifically, if you call compilation--ensure-parse up to point-max
from compilation-finish-functions, I can't think of any reason why
compilation-error-properties should be called afterwards.  If you can
get a backtrace of when that happens, it would help.

E.g. set a buffer-local var from compilation-finish-functions after
calling compilation--ensure-parse, and then add an assert in
compilation-error-properties to check that that var is not set yes.


        Stefan




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Sat, 08 Oct 2016 18:11:01 GMT) Full text and rfc822 format available.

Message #68 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 24585 <at> debbugs.gnu.org
Subject: Re: bug#24585: 25.1;
 avoid hack in ggtags.el to run compilation-auto-jump timer
Date: Sun, 09 Oct 2016 02:10:00 +0800
On 2016-10-07 14:10 -0400, Stefan Monnier wrote:
> Hmm... calling compilation--ensure-parse *should* help.
> More specifically, if you call compilation--ensure-parse up to point-max
> from compilation-finish-functions, I can't think of any reason why
> compilation-error-properties should be called afterwards.  If you can
> get a backtrace of when that happens, it would help.
>
> E.g. set a buffer-local var from compilation-finish-functions after
> calling compilation--ensure-parse, and then add an assert in
> compilation-error-properties to check that that var is not set yes.

To the best of my knowledge that seems to work with change like this to
compile.el:

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index f2e397a4..3a4f4b8b 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1052,13 +1052,18 @@ POS and RES.")
 	      l2
 	    (setcdr l1 (cons (list ,key) l2)))))))
 
-(defun compilation-auto-jump (buffer pos)
-  (with-current-buffer buffer
-    (goto-char pos)
-    (let ((win (get-buffer-window buffer 0)))
-      (if win (set-window-point win pos)))
-    (if compilation-auto-jump-to-first-error
-	(compile-goto-error))))
+(defvar-local compilation-pending-auto-jump nil)
+
+(defun compilation-auto-jump (buffer)
+  (when (buffer-live-p buffer)
+    (with-current-buffer buffer
+      (when compilation-pending-auto-jump
+        (goto-char compilation-pending-auto-jump)
+        (let ((win (get-buffer-window buffer 0)))
+          (if win (set-window-point win compilation-pending-auto-jump)))
+        (setq compilation-pending-auto-jump nil)
+        (if compilation-auto-jump-to-first-error
+            (compile-goto-error))))))
 
 ;; This function is the central driver, called when font-locking to gather
 ;; all information needed to later jump to corresponding source code.
@@ -1126,8 +1131,8 @@ POS and RES.")
     (when (and compilation-auto-jump-to-next
                (>= type compilation-skip-threshold))
       (kill-local-variable 'compilation-auto-jump-to-next)
-      (run-with-timer 0 nil 'compilation-auto-jump
-                      (current-buffer) (match-beginning 0)))
+      (setq compilation-pending-auto-jump (match-beginning 0))
+      (run-with-timer 0 nil 'compilation-auto-jump (current-buffer)))
 
     (compilation-internal-error-properties
      file line end-line col end-col type fmt)))




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 04 Sep 2020 12:48:01 GMT) Full text and rfc822 format available.

Message #71 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 24585 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#24585: 25.1; avoid hack in ggtags.el to run
 compilation-auto-jump timer
Date: Fri, 04 Sep 2020 14:47:03 +0200
Leo Liu <sdl.web <at> gmail.com> writes:

> On 2016-10-07 14:10 -0400, Stefan Monnier wrote:
>> Hmm... calling compilation--ensure-parse *should* help.
>> More specifically, if you call compilation--ensure-parse up to point-max
>> from compilation-finish-functions, I can't think of any reason why
>> compilation-error-properties should be called afterwards.  If you can
>> get a backtrace of when that happens, it would help.
>>
>> E.g. set a buffer-local var from compilation-finish-functions after
>> calling compilation--ensure-parse, and then add an assert in
>> compilation-error-properties to check that that var is not set yes.
>
> To the best of my knowledge that seems to work with change like this to
> compile.el:

[...]

This was the final message in this bug report, but looking at the patch
that supposedly fixes the problem, I don't understand how it makes a
difference: Putting (match-beginning) in a buffer-local variable
instead of passing it in directly seems like a no-op?

Checking whether the buffer is alive in compilation-auto-jump seems like
a good idea, though (any async function that switches to a buffer should
check whether it's alive first).

> -(defun compilation-auto-jump (buffer pos)
> -  (with-current-buffer buffer
> -    (goto-char pos)
> -    (let ((win (get-buffer-window buffer 0)))
> -      (if win (set-window-point win pos)))
> -    (if compilation-auto-jump-to-first-error
> -	(compile-goto-error))))
> +(defvar-local compilation-pending-auto-jump nil)
> +
> +(defun compilation-auto-jump (buffer)
> +  (when (buffer-live-p buffer)
> +    (with-current-buffer buffer
> +      (when compilation-pending-auto-jump
> +        (goto-char compilation-pending-auto-jump)
> +        (let ((win (get-buffer-window buffer 0)))
> +          (if win (set-window-point win compilation-pending-auto-jump)))
> +        (setq compilation-pending-auto-jump nil)
> +        (if compilation-auto-jump-to-first-error
> +            (compile-goto-error))))))
>
>  ;; This function is the central driver, called when font-locking to gather
>  ;; all information needed to later jump to corresponding source code.
> @@ -1126,8 +1131,8 @@ POS and RES.")
>      (when (and compilation-auto-jump-to-next
>                 (>= type compilation-skip-threshold))
>        (kill-local-variable 'compilation-auto-jump-to-next)
> -      (run-with-timer 0 nil 'compilation-auto-jump
> -                      (current-buffer) (match-beginning 0)))
> +      (setq compilation-pending-auto-jump (match-beginning 0))
> +      (run-with-timer 0 nil 'compilation-auto-jump (current-buffer)))
>
>      (compilation-internal-error-properties
>       file line end-line col end-col type fmt)))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 04 Sep 2020 14:30:02 GMT) Full text and rfc822 format available.

Message #74 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 24585 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#24585: 25.1; avoid hack in ggtags.el to run
 compilation-auto-jump timer
Date: Fri, 04 Sep 2020 10:29:15 -0400
> This was the final message in this bug report, but looking at the patch
> that supposedly fixes the problem, I don't understand how it makes a
> difference: Putting (match-beginning) in a buffer-local variable
> instead of passing it in directly seems like a no-op?

This was too long ago and got moved to write-only swap space, and
I haven't had time to dig into it now to recover the lost context, but
I can see a potential case where it makes a difference, which is if the
timer is set multiple times before it gets a chance to be run: with the
new code only one of the timers will actually be executed.

> Checking whether the buffer is alive in compilation-auto-jump seems like
> a good idea, though (any async function that switches to a buffer should
> check whether it's alive first).

Indeed,


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 04 Sep 2020 15:03:02 GMT) Full text and rfc822 format available.

Message #77 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 24585 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#24585: 25.1; avoid hack in ggtags.el to run
 compilation-auto-jump timer
Date: Fri, 04 Sep 2020 17:02:00 +0200
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> This was too long ago and got moved to write-only swap space, and
> I haven't had time to dig into it now to recover the lost context, but
> I can see a potential case where it makes a difference, which is if the
> timer is set multiple times before it gets a chance to be run: with the
> new code only one of the timers will actually be executed.

That's true; I didn't think about that.  But wouldn't a more obvious fix
be to cancel the previous timer, if there's already a timer in-flight?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Fri, 04 Sep 2020 15:46:02 GMT) Full text and rfc822 format available.

Message #80 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 24585 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#24585: 25.1; avoid hack in ggtags.el to run
 compilation-auto-jump timer
Date: Fri, 04 Sep 2020 11:45:31 -0400
>> This was too long ago and got moved to write-only swap space, and
>> I haven't had time to dig into it now to recover the lost context, but
>> I can see a potential case where it makes a difference, which is if the
>> timer is set multiple times before it gets a chance to be run: with the
>> new code only one of the timers will actually be executed.
>
> That's true; I didn't think about that.  But wouldn't a more obvious fix
> be to cancel the previous timer, if there's already a timer in-flight?

Could be.  I don't even know if this effect was the one intended in
any case.


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Sat, 05 Sep 2020 12:34:02 GMT) Full text and rfc822 format available.

Message #83 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 24585 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#24585: 25.1; avoid hack in ggtags.el to run
 compilation-auto-jump timer
Date: Sat, 05 Sep 2020 14:32:57 +0200
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>> This was too long ago and got moved to write-only swap space, and
>>> I haven't had time to dig into it now to recover the lost context, but
>>> I can see a potential case where it makes a difference, which is if the
>>> timer is set multiple times before it gets a chance to be run: with the
>>> new code only one of the timers will actually be executed.
>>
>> That's true; I didn't think about that.  But wouldn't a more obvious fix
>> be to cancel the previous timer, if there's already a timer in-flight?
>
> Could be.  I don't even know if this effect was the one intended in
> any case.

Me neither.  I've now applied the

+  (when (buffer-live-p buffer)

part of the patch, but not the rest.  Perhaps Leo can chime in with that
the patch was trying to achieve.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Added tag(s) moreinfo. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Sat, 05 Sep 2020 12:34:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#24585; Package emacs. (Wed, 07 Oct 2020 03:32:02 GMT) Full text and rfc822 format available.

Message #88 received at 24585 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 24585 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#24585: 25.1; avoid hack in ggtags.el to run
 compilation-auto-jump timer
Date: Wed, 07 Oct 2020 05:30:44 +0200
Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Me neither.  I've now applied the
>
> +  (when (buffer-live-p buffer)
>
> part of the patch, but not the rest.  Perhaps Leo can chime in with that
> the patch was trying to achieve.

This was a month ago, but there was no response, so I'm closing this bug
report.  If there's more to be done here, a new bug report could perhaps
be opened.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




bug closed, send any further explanations to 24585 <at> debbugs.gnu.org and Leo Liu <sdl.web <at> gmail.com> Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Wed, 07 Oct 2020 03:32:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 04 Nov 2020 12:24:10 GMT) Full text and rfc822 format available.

This bug report was last modified 3 years and 145 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.