Fix flymake to work with my Emacs, and hopefully not break others.

Fix for configs where the Chrome checkout is under user's home directory (in which case buffer-file-truename returns a path starting with ~).

BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113664

[Jói, 2011-12-08 13:24:37]

Hi Ami,

I finally got round to getting this working in my config, and there were a
couple of changes I needed to make, reflected here.

The path tricks weren't working with my Emacs that would provide e.g.
~/c/chrome/src/base/rand_util.cc as the buffer-file-truename variable, but using
absolute paths fixed it, and I don't think it should break things for other
Emacsen.

I wasn't sure about the defadvice change, let me know if it looks right to you.

Cheers,
Jói

[Ami GONE FROM CHROMIUM, 2011-12-08 16:17:01]

Welcome to the flymake party!

I suspect we can get you up & running without these changes; see below.

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
File tools/emacs/flymake-chromium.el (left):

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:71: (defadvice next-error (around
cr-flymake-next-error activate)
This change is wrong - that particular arg to defadvice is the _name_ of the
advice, not the function being advised (that'd be next-error, defadvice's first
arg).
(update CL description after reverting this)

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
File tools/emacs/flymake-chromium.el (right):

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:23: Emacsen, buffer-file-truename is not
absolute but rather uses a
I believe this behavior is in fact the case for all emacsen.

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:46: (substring (cr-flymake-absbufferpath)
(length (cr-flymake-chromium-src)))))
I don't understand the need for these abspath changes.
locate-dominating-file works fine for me with ~-abreviated paths, so this
function correctly returns the relative path from the src root to the current
buffer's file.

Can you say more about what triggered the need for this change?  Specifically
what OS/emacs version are you on, and can you give me an example file you were
editing that caused this function to barf (and how, if you have it)?
Happy to pair-debug over IM when I'm in the office.

[Jói, 2011-12-08 16:24:35]

Hi Ami,

OK, misunderstood the defadvice thing, will revert that bit.

There were two things that prompted the abspath change:

a) Without it, the relative path for the .o file to build would be
something like ../../../../base/rand_util.o^ instead of the correct
../../base/rand_util.o^.  With a full absolute path, it would
calculate it correctly.

b) ninja would report an error changing directory to
~/c/chrome/src/out/Debug even though the directory exists; with a full
absolute path it would not.

I'm on stock Ubuntu 11.10, and my Emacs is GNU Emacs 23.3.1.

Cheers,
Jói



On Thu, Dec 8, 2011 at 4:17 PM,  <fischman@chromium.org> wrote:
> Welcome to the flymake party!
>
> I suspect we can get you up & running without these changes; see below.
>
>
>
http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
> File tools/emacs/flymake-chromium.el (left):
>
>
http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
> tools/emacs/flymake-chromium.el:71: (defadvice next-error (around
> cr-flymake-next-error activate)
> This change is wrong - that particular arg to defadvice is the _name_ of
> the advice, not the function being advised (that'd be next-error,
> defadvice's first arg).
> (update CL description after reverting this)
>
>
http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
> File tools/emacs/flymake-chromium.el (right):
>
>
http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
> tools/emacs/flymake-chromium.el:23: Emacsen, buffer-file-truename is not
>
> absolute but rather uses a
> I believe this behavior is in fact the case for all emacsen.
>
>
http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
> tools/emacs/flymake-chromium.el:46: (substring
> (cr-flymake-absbufferpath) (length (cr-flymake-chromium-src)))))
> I don't understand the need for these abspath changes.
> locate-dominating-file works fine for me with ~-abreviated paths, so
> this function correctly returns the relative path from the src root to
> the current buffer's file.
>
> Can you say more about what triggered the need for this change?
> Specifically what OS/emacs version are you on, and can you give me an
> example file you were editing that caused this function to barf (and
> how, if you have it)?
> Happy to pair-debug over IM when I'm in the office.
>
> http://codereview.chromium.org/8872013/

[Ami GONE FROM CHROMIUM, 2011-12-08 19:03:03]

I suspect both of your problems stem from the same place.
I can't repro the problem starting from an emacs -Q instead (and manually
loading flymake-chromium.el), with emacs 23.2.  Can you repro it starting from a
-Q session?
I'm currently building 23.3 to see if I can repro.

[Ami GONE FROM CHROMIUM, 2011-12-08 19:27:49]

Now I see the difference - my chromium src lives on local disk but I open it
from a symlink under ~/, which causes buffer-file-truename to return a path free
of ~ anyway (b/c of symlink resolution, the point of "truename").  So in my
setup paths are absolutified by truename, but in yours they aren't.  Oops.

Modulo the comments below, LGTM.
Thanks for doing this!

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
File tools/emacs/flymake-chromium.el (right):

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:23: Emacsen, buffer-file-truename is not
absolute but rather uses a
Remove the second sentence of the docstring.

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:32: (when srcdir (expand-file-name srcdir))))
You don't need this when (and thus the above let) b/c cr-flymake-absbufferpath
already has a guard.

[Jói, 2011-12-08 20:46:16]

Thanks Ami.  flymake+ninja totally rocks so I'm really happy to have it working
now.

I've addressed two of the three issues you pointed out.  See explanation for the
third below.

I also updated the change description.

Cheers,
Jói

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
File tools/emacs/flymake-chromium.el (right):

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:23: Emacsen, buffer-file-truename is not
absolute but rather uses a
On 2011/12/08 19:27:49, Ami Fischman wrote:
> Remove the second sentence of the docstring.

Done.

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:32: (when srcdir (expand-file-name srcdir))))
On 2011/12/08 19:27:49, Ami Fischman wrote:
> You don't need this when (and thus the above let) b/c cr-flymake-absbufferpath
> already has a guard.

locate-dominating-file is fine with a nil and then returns a nil, but it returns
a ~ prefixed path for my config when it's non-nil, so I need expand-file-name so
we can do the prefix checking correctly.

Since expand-file-name errors out if you pass it a nil, I think I still need the
guard here.

[Ami GONE FROM CHROMIUM, 2011-12-08 20:49:16]

Still LGTM.

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium.el
File tools/emacs/flymake-chromium.el (right):

http://codereview.chromium.org/8872013/diff/6001/tools/emacs/flymake-chromium...
tools/emacs/flymake-chromium.el:32: (when srcdir (expand-file-name srcdir))))
On 2011/12/08 20:46:16, Jói wrote:
> On 2011/12/08 19:27:49, Ami Fischman wrote:
> > You don't need this when (and thus the above let) b/c
cr-flymake-absbufferpath
> > already has a guard.
> 
> locate-dominating-file is fine with a nil and then returns a nil, but it
returns
> a ~ prefixed path for my config when it's non-nil, so I need expand-file-name
so
> we can do the prefix checking correctly.
> 
> Since expand-file-name errors out if you pass it a nil, I think I still need
the
> guard here.

I was mis-reading the code before (thinking you're passing srcdir to
cr-flymake-absbufferpath).  You're right.