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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

garyo
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                [hidden email]
GenArts, Inc.                   Tel: 617-492-2888
955 Mass. Ave                   Fax: 617-492-2852
Cambridge, MA 02139 USA         www.genarts.com




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Hyatt-2

Gary Oberbrunner <[hidden email]> 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?



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eli Zaretskii
> From: Andrew Hyatt <[hidden email]>
> Date: Tue, 26 Jan 2016 00:21:51 -0500
> Cc: [hidden email]
>
> Gary Oberbrunner <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

garyo
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" <[hidden email]>
> To: "Andrew Hyatt" <[hidden email]>
> Cc: "Gary Oberbrunner" <[hidden email]>, [hidden email]
> 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 <[hidden email]>
>> Date: Tue, 26 Jan 2016 00:21:51 -0500
>> Cc: [hidden email]
>>
>> Gary Oberbrunner <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]>
> Cc: Andrew Hyatt <[hidden email]>, [hidden email]
>
> 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?



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

garyo
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" <[hidden email]>
> To: "Gary Oberbrunner" <[hidden email]>
> Cc: "Andrew Hyatt" <[hidden email]>, "3418" <[hidden email]>
> 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 <[hidden email]>
>> Cc: Andrew Hyatt <[hidden email]>, [hidden email]
>>
>> 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



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noam Postavsky-2
Gary Oberbrunner <[hidden email]> 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)))



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

garyo
This sounds perfect to me.

On Thu, Aug 10, 2017 at 8:50 PM, <[hidden email]> wrote:
Gary Oberbrunner <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eli Zaretskii
In reply to this post by Noam Postavsky-2
> From: [hidden email]
> Cc: Eli Zaretskii <[hidden email]>,  [hidden email],  Andrew Hyatt <[hidden email]>
> Date: Thu, 10 Aug 2017 20:50:29 -0400
>
> Gary Oberbrunner <[hidden email]> 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?



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noam Postavsky-2
Eli Zaretskii <[hidden email]> 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.




Loading...