Fix the YCM tests on Mac.

Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This
tripped up our YCM config and tests, which resolved symlinks with
os.path.realpath in a few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit
removes every use of os.path.realpath and makes things happy.

BUG=643286
Committed: https://crrev.com/50d5177ecba5f3662f899a1972739571d25ffcc2
Cr-Commit-Position: refs/heads/master@{#417718}

[Sidney San Martín, 2016-09-01 17:52:58]

sdy@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2016-09-01 18:20:58]

https://codereview.chromium.org/2303813002/diff/1/tools/vim/tests/chromium.yc...
File tools/vim/tests/chromium.ycm_extra_conf_unittest.py (right):

https://codereview.chromium.org/2303813002/diff/1/tools/vim/tests/chromium.yc...
tools/vim/tests/chromium.ycm_extra_conf_unittest.py:77: self.chrome_root =
os.path.abspath(os.path.realpath(
Why is this necessary on Mac?

[Sidney San Martín, 2016-09-01 22:12:51]

Patchset #2 (id:20001) has been deleted

[Sidney San Martín, 2016-09-01 22:18:04]

https://codereview.chromium.org/2303813002/diff/1/tools/vim/tests/chromium.yc...
File tools/vim/tests/chromium.ycm_extra_conf_unittest.py (right):

https://codereview.chromium.org/2303813002/diff/1/tools/vim/tests/chromium.yc...
tools/vim/tests/chromium.ycm_extra_conf_unittest.py:77: self.chrome_root =
os.path.abspath(os.path.realpath(
On 2016/09/01 18:20:58, asanka wrote:
> Why is this necessary on Mac?

The temp directories on Mac are symlinks (/tmp -> /private/tmp, /var ->
/private/var), and that triggers a couple of bugs in these scripts. The first
one relates to ninja_output.GetNinjaOutputDirectory(), which performs some path
transformations on lines 40-42:

    out = os.path.realpath(os.path.join(chrome_root, f))
    if os.path.isdir(out):
      output_dirs.append(os.path.relpath(out, start = chrome_root))

Those path transformations go like this:

    os.path.join('/tmp/chrome_root359/', 'out')
      -> '/tmp/chrome_root359/out'
    os.path.realpath('/tmp/chrome_root359/out')
      -> '/private/tmp/chrome_root359/out'
    os.path.relpath('/private/tmp/chrome_root359/out', start =
'/tmp/chrome_root359/')
      -> '../../private/tmp/chrome_root359/out'

Later, on line 46:

    os.path.join('/tmp/chrome_root359', '../../private/tmp/chrome_root359/out')
      -> '/tmp/chrome_root359/../../private/tmp/chrome_root359/out'

The os.listdir() on line 47 follows each .. link and effectively tries to list:

    /private/private/tmp/chrome_root359/out

…and throws. I fixed this in a new patchset, but there's still a second issue.
Chromium_ycmExtraConfTest.NormalizeString() looks like this:

    replace(self.chrome_root, '[SRC]').replace('\\', '/')

When self.chrome_root includes a symlink, the path returned by Ninja will have
resolved that symlink, so the replacement will fail (or, in this case, miss the
/private at the beginning and return "/private[SRC]"). This particular change to
resolve symlinks up front resolves that. I moved it to a better spot in the new
patchset, though.

[asanka, 2016-09-02 15:03:21]

On 2016/09/01 at 22:18:04, sdy wrote:
>
https://codereview.chromium.org/2303813002/diff/1/tools/vim/tests/chromium.yc...
> File tools/vim/tests/chromium.ycm_extra_conf_unittest.py (right):
> 
>
https://codereview.chromium.org/2303813002/diff/1/tools/vim/tests/chromium.yc...
> tools/vim/tests/chromium.ycm_extra_conf_unittest.py:77: self.chrome_root =
os.path.abspath(os.path.realpath(
> On 2016/09/01 18:20:58, asanka wrote:
> > Why is this necessary on Mac?
> 
> The temp directories on Mac are symlinks (/tmp -> /private/tmp, /var ->
/private/var), and that triggers a couple of bugs in these scripts. The first
one relates to ninja_output.GetNinjaOutputDirectory(), which performs some path
transformations on lines 40-42:
> 
>     out = os.path.realpath(os.path.join(chrome_root, f))
>     if os.path.isdir(out):
>       output_dirs.append(os.path.relpath(out, start = chrome_root))
> 
> Those path transformations go like this:
> 
>     os.path.join('/tmp/chrome_root359/', 'out')
>       -> '/tmp/chrome_root359/out'
>     os.path.realpath('/tmp/chrome_root359/out')
>       -> '/private/tmp/chrome_root359/out'
>     os.path.relpath('/private/tmp/chrome_root359/out', start =
'/tmp/chrome_root359/')
>       -> '../../private/tmp/chrome_root359/out'
> 
> Later, on line 46:
> 
>     os.path.join('/tmp/chrome_root359',
'../../private/tmp/chrome_root359/out')
>       -> '/tmp/chrome_root359/../../private/tmp/chrome_root359/out'
> 
> The os.listdir() on line 47 follows each .. link and effectively tries to
list:
> 
>     /private/private/tmp/chrome_root359/out
> 
> …and throws. I fixed this in a new patchset, but there's still a second issue.
Chromium_ycmExtraConfTest.NormalizeString() looks like this:
> 
>     replace(self.chrome_root, '[SRC]').replace('\\', '/')
> 
> When self.chrome_root includes a symlink, the path returned by Ninja will have
resolved that symlink, so the replacement will fail (or, in this case, miss the
/private at the beginning and return "/private[SRC]"). This particular change to
resolve symlinks up front resolves that. I moved it to a better spot in the new
patchset, though.

Got it.

[asanka, 2016-09-02 15:05:14]

Let's also fix GetNinjaOutputsForSourceFile to not rely on there not being
symlinks in its input paths.

https://codereview.chromium.org/2303813002/diff/40001/tools/vim/tests/chromiu...
File tools/vim/tests/chromium.ycm_extra_conf_unittest.py (right):

https://codereview.chromium.org/2303813002/diff/40001/tools/vim/tests/chromiu...
tools/vim/tests/chromium.ycm_extra_conf_unittest.py:110: self.test_root =
os.path.realpath(tempfile.mkdtemp())
Shall we avoid calling realpath() here and try to get rid of .relpath() uses
elsewhere? I think we could end up masking similar bugs if we call realpath()
here. Optionally I'd suggest explicitly setting things up so that self.test_root
contains a symlink somewhere so that we can smoke out similar issues.

[Sidney San Martín, 2016-09-09 14:50:52]

Description was changed from

==========
Fix the YCM tests on Mac.

BUG=643286
==========

to

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This tripped
up our YCM config and tests, which resolved symlinks with os.path.realpath in a
few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit removes every
use of os.path.realpath and makes things happy.

BUG=643286
==========

[Sidney San Martín, 2016-09-09 14:51:23]

Description was changed from

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This tripped
up our YCM config and tests, which resolved symlinks with os.path.realpath in a
few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit removes every
use of os.path.realpath and makes things happy.

BUG=643286
==========

to

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This
tripped up our YCM config and tests, which resolved symlinks with
os.path.realpath in a few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit
removes every use of os.path.realpath and makes things happy.

BUG=643286
==========

[Sidney San Martín, 2016-09-09 14:55:49]

Thanks for waiting! I wanted to come back to this when I had some time to come
up with a proper fix.

https://codereview.chromium.org/2303813002/diff/40001/tools/vim/tests/chromiu...
File tools/vim/tests/chromium.ycm_extra_conf_unittest.py (right):

https://codereview.chromium.org/2303813002/diff/40001/tools/vim/tests/chromiu...
tools/vim/tests/chromium.ycm_extra_conf_unittest.py:110: self.test_root =
os.path.realpath(tempfile.mkdtemp())
On 2016/09/02 15:05:13, asanka wrote:
> Shall we avoid calling realpath() here and try to get rid of .relpath() uses
> elsewhere? I think we could end up masking similar bugs if we call realpath()
> here. Optionally I'd suggest explicitly setting things up so that
self.test_root
> contains a symlink somewhere so that we can smoke out similar issues.

I like it :). See the latest patch set — some uses of relpath are useful, like
getting the path of a file relative to the out dir, but it turns out that
realpath was *not* useful.

[asanka, 2016-09-09 20:05:47]

lgtm. thanks!

[Sidney San Martín, 2016-09-09 20:43:25]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2016-09-09 20:44:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 21:37:10]

Description was changed from

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This
tripped up our YCM config and tests, which resolved symlinks with
os.path.realpath in a few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit
removes every use of os.path.realpath and makes things happy.

BUG=643286
==========

to

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This
tripped up our YCM config and tests, which resolved symlinks with
os.path.realpath in a few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit
removes every use of os.path.realpath and makes things happy.

BUG=643286
==========

[commit-bot: I haz the power, 2016-09-09 21:37:11]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-09 21:39:24]

Description was changed from

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This
tripped up our YCM config and tests, which resolved symlinks with
os.path.realpath in a few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit
removes every use of os.path.realpath and makes things happy.

BUG=643286
==========

to

==========
Fix the YCM tests on Mac.

macOS temporary directories (/tmp and /var/folders) are symlinks. This
tripped up our YCM config and tests, which resolved symlinks with
os.path.realpath in a few spots but not everywhere.

Resolving symlinks doesn't seem to buy us anything, so this commit
removes every use of os.path.realpath and makes things happy.

BUG=643286

Committed: https://crrev.com/50d5177ecba5f3662f899a1972739571d25ffcc2
Cr-Commit-Position: refs/heads/master@{#417718}
==========

[commit-bot: I haz the power, 2016-09-09 21:39:24]

Patchset 3 (id:??) landed as
https://crrev.com/50d5177ecba5f3662f899a1972739571d25ffcc2
Cr-Commit-Position: refs/heads/master@{#417718}