bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Gary Oberbrunner]

Hi emacs folks.  I submitted a patch to compilation-get-file-structure 
in compile.el in 2001, introducing this stanza:

	;; If compilation-parse-errors-filename-function is
	;; defined, use it to process the filename.
	(when compilation-parse-errors-filename-function
	  (setq filename
		(funcall
			 filename)))

At some point since then, the filename was changed to not always be 
absolute; there's now a variable spec-directory in that function.  This 
means that implementations of compilation-parse-errors-filename-function 
can't always work correctly since it doesn't know the full path of the file.

I'm happy to work on a fix, but I see a few issues.

Solution 1: add 2nd arg SPEC-DIRECTORY to 
compilation-parse-errors-filename-function.
Problem: existing implementations will get an incorrect number of args 
error and will have to change.

Solution 2: make filename absolute before passing to 
compilation-parse-errors-filename-function.
Problem: the rest of the code is pretty careful not to absolutize the 
filename; this would change the behavior in ways I don't completely 
understand.

Of course I am personally happy with solution 1, but since it affects 
compatibility I thought I should bring it up on this list.  I am not on 
the list, so please cc me with any replies, thanks!

-- 
. . . . . . . . . . . . . . . . . . . . . . . . .
Gary Oberbrunner                garyo@genarts.com
GenArts, Inc.                   Tel: 617-492-2888
955 Mass. Ave                   Fax: 617-492-2852
Cambridge, MA 02139 USA         www.genarts.com

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Andrew Hyatt]

Gary Oberbrunner <garyo@genarts.com> writes:

> Hi emacs folks.  I submitted a patch to compilation-get-file-structure in
> compile.el in 2001, introducing this stanza:
>
> 	;; If compilation-parse-errors-filename-function is
> 	;; defined, use it to process the filename.
> 	(when compilation-parse-errors-filename-function
> 	  (setq filename
> 		(funcall
> 			 filename)))
>
> At some point since then, the filename was changed to not always be absolute;
> there's now a variable spec-directory in that function.  This means that
> implementations of compilation-parse-errors-filename-function can't always work
> correctly since it doesn't know the full path of the file.
>
> I'm happy to work on a fix, but I see a few issues.
>
> Solution 1: add 2nd arg SPEC-DIRECTORY to
> compilation-parse-errors-filename-function.
> Problem: existing implementations will get an incorrect number of args error and
> will have to change.
>
> Solution 2: make filename absolute before passing to
> compilation-parse-errors-filename-function.
> Problem: the rest of the code is pretty careful not to absolutize the filename;
> this would change the behavior in ways I don't completely understand.
>
> Of course I am personally happy with solution 1, but since it affects
> compatibility I thought I should bring it up on this list.  I am not on the
> list, so please cc me with any replies, thanks!

Sadly, this bug hasn't been responded to.  Your description is pretty
code-intensive, for those of us not familiar with the internals, can you
give instructions on how to reproduce a user-visible issue?

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Eli Zaretskii]

> From: Andrew Hyatt <ahyatt@gmail.com>
> Date: Tue, 26 Jan 2016 00:21:51 -0500
> Cc: 3418@debbugs.gnu.org
> 
> Gary Oberbrunner <garyo@genarts.com> writes:
> 
> > Hi emacs folks.  I submitted a patch to compilation-get-file-structure in
> > compile.el in 2001, introducing this stanza:
> >
> > 	;; If compilation-parse-errors-filename-function is
> > 	;; defined, use it to process the filename.
> > 	(when compilation-parse-errors-filename-function
> > 	  (setq filename
> > 		(funcall
> > 			 filename)))
> >
> > At some point since then, the filename was changed to not always be absolute;
> > there's now a variable spec-directory in that function.  This means that
> > implementations of compilation-parse-errors-filename-function can't always work
> > correctly since it doesn't know the full path of the file.
> >
> > I'm happy to work on a fix, but I see a few issues.
> >
> > Solution 1: add 2nd arg SPEC-DIRECTORY to
> > compilation-parse-errors-filename-function.
> > Problem: existing implementations will get an incorrect number of args error and
> > will have to change.
> >
> > Solution 2: make filename absolute before passing to
> > compilation-parse-errors-filename-function.
> > Problem: the rest of the code is pretty careful not to absolutize the filename;
> > this would change the behavior in ways I don't completely understand.
> >
> > Of course I am personally happy with solution 1, but since it affects
> > compatibility I thought I should bring it up on this list.  I am not on the
> > list, so please cc me with any replies, thanks!
> 
> Sadly, this bug hasn't been responded to.  Your description is pretty
> code-intensive, for those of us not familiar with the internals, can you
> give instructions on how to reproduce a user-visible issue?

FWIW, I don't see why not adopt Soution 1, just make the second
argument optional.  That would be backward-compatible, IIUC.

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Gary Oberbrunner]

Wow, a blast from the past! 

I am totally happy with soln 1. For all I know, since I added that hook I might be the only one using it. :-) But I'm also usually a stickler for backward compatibility, so that's why I brought it up.

As for how to make it happen, I'm not sure what triggers the absolute vs. relative names getting passed around in compilation-parse-errors; it probably depends on what it finds in the *compilation* buffer. But Andrew, in order for this to matter at all, the emacs user has to have added their own compilation-parse-errors-filename-function, so "normal" users wouldn't be affected by this at all.

The use case is a build system that copies/symlinks the sources to a build dir and compiles them there rather than in their original location. Compilation errors will be given relative to that build dir, not the original source. A user with such a build system would make a compilation-parse-errors-filename-function that would string-replace the build dir to the source dir, so next-error would jump to the proper source file (not the build dir copy).

----- Original Message -----
> From: "Eli Zaretskii" <eliz@gnu.org>
> To: "Andrew Hyatt" <ahyatt@gmail.com>
> Cc: "Gary Oberbrunner" <garyo@genarts.com>, 3418@debbugs.gnu.org
> Sent: Tuesday, January 26, 2016 9:42:58 AM
> Subject: Re: bug#3418: Issue with compile.el and	compilation-parse-errors-filename-function

>> From: Andrew Hyatt <ahyatt@gmail.com>
>> Date: Tue, 26 Jan 2016 00:21:51 -0500
>> Cc: 3418@debbugs.gnu.org
>> 
>> Gary Oberbrunner <garyo@genarts.com> writes:
>> 
>> > Hi emacs folks.  I submitted a patch to compilation-get-file-structure in
>> > compile.el in 2001, introducing this stanza:
>> >
>> > 	;; If compilation-parse-errors-filename-function is
>> > 	;; defined, use it to process the filename.
>> > 	(when compilation-parse-errors-filename-function
>> > 	  (setq filename
>> > 		(funcall
>> > 			 filename)))
>> >
>> > At some point since then, the filename was changed to not always be absolute;
>> > there's now a variable spec-directory in that function.  This means that
>> > implementations of compilation-parse-errors-filename-function can't always work
>> > correctly since it doesn't know the full path of the file.
>> >
>> > I'm happy to work on a fix, but I see a few issues.
>> >
>> > Solution 1: add 2nd arg SPEC-DIRECTORY to
>> > compilation-parse-errors-filename-function.
>> > Problem: existing implementations will get an incorrect number of args error and
>> > will have to change.
>> >
>> > Solution 2: make filename absolute before passing to
>> > compilation-parse-errors-filename-function.
>> > Problem: the rest of the code is pretty careful not to absolutize the filename;
>> > this would change the behavior in ways I don't completely understand.
>> >
>> > Of course I am personally happy with solution 1, but since it affects
>> > compatibility I thought I should bring it up on this list.  I am not on the
>> > list, so please cc me with any replies, thanks!
>> 
>> Sadly, this bug hasn't been responded to.  Your description is pretty
>> code-intensive, for those of us not familiar with the internals, can you
>> give instructions on how to reproduce a user-visible issue?
> 
> FWIW, I don't see why not adopt Soution 1, just make the second
> argument optional.  That would be backward-compatible, IIUC.

-- 
Gary Oberbrunner

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Eli Zaretskii]

> Date: Tue, 26 Jan 2016 10:15:27 -0500 (EST)
> From: Gary Oberbrunner <garyo@genarts.com>
> Cc: Andrew Hyatt <ahyatt@gmail.com>, 3418@debbugs.gnu.org
> 
> Wow, a blast from the past! 

Better late than never, right?

> I am totally happy with soln 1. For all I know, since I added that hook I might be the only one using it. :-) But I'm also usually a stickler for backward compatibility, so that's why I brought it up.

But if we make the additional argument optional, the backward
compatibility is preserved, right?  Or did I miss something?

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Gary Oberbrunner]

Hi, Eli. 
If a user (such as myself) has an implementation of this function in his .emacs today, like so:

(defun process-error-filename (filename)
  ;;; do stuff with filename
  filename)
(setq compilation-parse-errors-filename-function 'process-error-filename)

and we add a new argument that gets passed to that function, it'll throw an error. *Users* will have to add 
  &optional spec-dir 
to their implementations of it to avoid the error.

(And btw, I've already done that in mine, so I'm future-proof. :-))

-- Gary

----- Original Message -----
> From: "Eli Zaretskii" <eliz@gnu.org>
> To: "Gary Oberbrunner" <garyo@genarts.com>
> Cc: "Andrew Hyatt" <ahyatt@gmail.com>, "3418" <3418@debbugs.gnu.org>
> Sent: Tuesday, January 26, 2016 11:08:50 AM
> Subject: Re: bug#3418: Issue with compile.el and	compilation-parse-errors-filename-function

>> Date: Tue, 26 Jan 2016 10:15:27 -0500 (EST)
>> From: Gary Oberbrunner <garyo@genarts.com>
>> Cc: Andrew Hyatt <ahyatt@gmail.com>, 3418@debbugs.gnu.org
>> 
>> Wow, a blast from the past!
> 
> Better late than never, right?
> 
>> I am totally happy with soln 1. For all I know, since I added that hook I might
>> be the only one using it. :-) But I'm also usually a stickler for backward
>> compatibility, so that's why I brought it up.
> 
> But if we make the additional argument optional, the backward
> compatibility is preserved, right?  Or did I miss something?

-- 
Gary Oberbrunner

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[npostavs]

Gary Oberbrunner <garyo@genarts.com> writes:

> If a user (such as myself) has an implementation of this function in his .emacs today, like so:
>
> (defun process-error-filename (filename)
>   ;;; do stuff with filename
>   filename)
> (setq compilation-parse-errors-filename-function 'process-error-filename)
>
> and we add a new argument that gets passed to that function, it'll throw an error. *Users* will have to add 
>   &optional spec-dir 
> to their implementations of it to avoid the error.

We could do something like

    (condition-case err
        (funcall compilation-parse-errors-filename-function filename spec-dir)
      (wrong-number-of-arguments
       ;; Try again with single arg for backwards compatibility.
       (funcall compilation-parse-errors-filename-function filename)))

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Gary Oberbrunner]

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

This sounds perfect to me.

On Thu, Aug 10, 2017 at 8:50 PM, <npostavs@users.sourceforge.net> wrote:

> Gary Oberbrunner <garyo@genarts.com> writes:
>
> > If a user (such as myself) has an implementation of this function in his
> .emacs today, like so:
> >
> > (defun process-error-filename (filename)
> >   ;;; do stuff with filename
> >   filename)
> > (setq compilation-parse-errors-filename-function
> 'process-error-filename)
> >
> > and we add a new argument that gets passed to that function, it'll throw
> an error. *Users* will have to add
> >   &optional spec-dir
> > to their implementations of it to avoid the error.
>
> We could do something like
>
>     (condition-case err
>         (funcall compilation-parse-errors-filename-function filename
> spec-dir)
>       (wrong-number-of-arguments
>        ;; Try again with single arg for backwards compatibility.
>        (funcall compilation-parse-errors-filename-function filename)))
>



-- 
Gary Oberbrunner *--* CTO *--* Boris FX

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

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: Eli Zaretskii <eliz@gnu.org>,  3418@debbugs.gnu.org,  Andrew Hyatt <ahyatt@gmail.com>
> Date: Thu, 10 Aug 2017 20:50:29 -0400
> 
> Gary Oberbrunner <garyo@genarts.com> writes:
> 
> > If a user (such as myself) has an implementation of this function in his .emacs today, like so:
> >
> > (defun process-error-filename (filename)
> >   ;;; do stuff with filename
> >   filename)
> > (setq compilation-parse-errors-filename-function 'process-error-filename)
> >
> > and we add a new argument that gets passed to that function, it'll throw an error. *Users* will have to add 
> >   &optional spec-dir 
> > to their implementations of it to avoid the error.
> 
> We could do something like
> 
>     (condition-case err
>         (funcall compilation-parse-errors-filename-function filename spec-dir)
>       (wrong-number-of-arguments
>        ;; Try again with single arg for backwards compatibility.
>        (funcall compilation-parse-errors-filename-function filename)))

Or use func-arity?

bug#3418: Issue with compile.el and compilation-parse-errors-filename-function

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> We could do something like
>> 
>>     (condition-case err
>>         (funcall compilation-parse-errors-filename-function filename spec-dir)
>>       (wrong-number-of-arguments
>>        ;; Try again with single arg for backwards compatibility.
>>        (funcall compilation-parse-errors-filename-function filename)))
>
> Or use func-arity?

I think func-arity could fail in case the function is advised or created
with `apply-partially'.  On the other hand the condition-case trick can
cause problems if there is an unrelated wrong arguments error inside the
function (perhaps this can be migitated by checking the error
information).  Doesn't really matter too much either way I guess, it's
all minor corner cases.