ninja windows: fix expansion of some VS macros

ninja windows: fix expansion of some VS macros

Fix expansion of $(OutDir). Also, make sure there's no redundant path contents (.., etc.) when expanding VS macros + gyp variables in the paths. Ninja can be more efficient if it doesn't need to re-canonicalize paths.

(removed the 'normalization' of converting to lower case that was previously in this CL)

R=thakis@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1343

[scottmg, 2012-04-25 23:14:10]

Hmm, breaks some stuff. Ignore me for now!

[Nico, 2012-04-25 23:18:23]

Too late!

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:203: def _WinCase(self, path):
Method name is improvable, also docstring

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:205: path = path.lower()
Maybe kernel32.GetLongPathName(kernel32.GetShortPathName(file_path)) to get the
"native" case?

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:510: os.path.normpath(os.path.relpath(qualified,
self.build_dir)))
What does this do?

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:1215: def wincase(path):
Make the function at the top global, then you don't need this. Maybe even move
it to msvs_emul

http://chromiumcodereview.appspot.com/10228016/diff/1/test/ninja/normalize-pa...
File test/ninja/normalize-paths-win/gyptest-normalize-paths.py (right):

http://chromiumcodereview.appspot.com/10228016/diff/1/test/ninja/normalize-pa...
test/ninja/normalize-paths-win/gyptest-normalize-paths.py:17: # case. Because
the file system is case-preserving, but insensitive, we have
This is true on Mac too. Why is this a problem on Windows, but not on Mac.

http://chromiumcodereview.appspot.com/10228016/diff/1/test/ninja/normalize-pa...
test/ninja/normalize-paths-win/gyptest-normalize-paths.py:20: # for efficient
stating on Windows.
Does windows have directory symlinks?

[M-A Ruel, 2012-04-26 01:08:49]

I'd prefer you to use the real file case instead of normalizing to lower case.

You can get the real file case with a combination of
GetLongPathName(GetShortPathName(path)). Yeah, this sounds stupid but it works.

See get_native_path_case() in:
http://src.chromium.org/viewvc/chrome/trunk/src/tools/isolate/trace_inputs.py...

[scottmg, 2012-04-26 03:06:46]

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:203: def _WinCase(self, path):
On 2012/04/25 23:18:23, Nico wrote:
> Method name is improvable, also docstring

Turns out it already exists as os.path.normcase!

http://chromiumcodereview.appspot.com/10228016/diff/1/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:510: os.path.normpath(os.path.relpath(qualified,
self.build_dir)))
On 2012/04/25 23:18:23, Nico wrote:
> What does this do?

Makes one's head hurt. :/

It was trying to be a build-dir-relative-normpath. Because the actions run in
the base dir, they have to be gyp->ninja'd, and when the paths contain VS
$(OutDir) macros, they ended up looking like "..\..\out\Debug\BLAH", when they
should just be "BLAH".  So this function was compensating for that. But, a much
better fix is to not retrieve a base-relative environment above instead (but
instead make it build-relative). So, it's gone now, thankfully.

In short: OMG WAT.

Also added to test for this.

http://chromiumcodereview.appspot.com/10228016/diff/1/test/ninja/normalize-pa...
File test/ninja/normalize-paths-win/gyptest-normalize-paths.py (right):

http://chromiumcodereview.appspot.com/10228016/diff/1/test/ninja/normalize-pa...
test/ninja/normalize-paths-win/gyptest-normalize-paths.py:17: # case. Because
the file system is case-preserving, but insensitive, we have
On 2012/04/25 23:18:23, Nico wrote:
> This is true on Mac too. Why is this a problem on Windows, but not on Mac.

Good question. I guess tools are generally more sensible on Mac. For example,
the output of cl's showIncludes has randomly cased drives (c: and C:), sometimes
"program files", and sometimes "Program Files", etc.

http://chromiumcodereview.appspot.com/10228016/diff/1/test/ninja/normalize-pa...
test/ninja/normalize-paths-win/gyptest-normalize-paths.py:20: # for efficient
stating on Windows.
On 2012/04/25 23:18:23, Nico wrote:
> Does windows have directory symlinks?

Yes.

[scottmg, 2012-04-26 03:56:37]

On 2012/04/26 01:08:49, Marc-Antoine Ruel wrote:
> I'd prefer you to use the real file case instead of normalizing to lower case.

OK. Any reason?

I agree that it's generally nicer to look at, but it causes a bit of a mess to
normalize to "disk case".

- The rule for how ninja should expect its input is "should be same as disk"?
That's pretty vague on Windows.
- git does the wrong thing by default on Windows (adding "A" does nothing if "a"
already exists).
- goma will have a hard time caching across machines if any paths (system, etc)
are different across machines.
- It means a disk access (or 2) per normalization, rather than just string
processing.
- 'cl /showIncludes' doesn't output in disk case, it's random: excerpt of some
lines from cl /showIncludes of a file that just does #include <windows.h>

Note: including file: C:\Program Files (x86)\Microsoft
SDKs\Windows\v7.0A\include\windows.h
Note: including file:  c:\Program Files (x86)\Microsoft Visual Studio
10.0\VC\INCLUDE\excpt.h
Note: including file:     c:\program files (x86)\microsoft visual studio
10.0\vc\include\codeanalysis\sourceannotations.h
Note: including file:    c:\program files (x86)\microsoft
sdks\windows\v7.0a\include\pshpack4.h
Note: including file:   C:\Program Files (x86)\Microsoft
SDKs\Windows\v7.0A\include\ime_cmodes.h

(Different case on drives, different case for VC\INCLUDE, different case for
program files (x86), etc.)

Anyway, overall the reason I like lower better is that it's one simple rule
that's easy to verify, rather than dependent on local state.

[M-A Ruel, 2012-04-26 15:57:40]

On 2012/04/26 03:56:37, scottmg wrote:
> On 2012/04/26 01:08:49, Marc-Antoine Ruel wrote:
> > I'd prefer you to use the real file case instead of normalizing to lower
case.
> 
> OK. Any reason?

When tracing inputs, I need to keep the case to be consistent across all the
OSes. For example, when mapping files to run unit_tests, I need to map the
following on all OSes: 
test/data/extensions/bad/Extensions/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/Current
Version

and this file on OSX:
<(PRODUCT_DIR)/Chromium Framework.framework/Versions/A/Resources/aliasCursor.png

These two are just examples, there's several of them. both svn and git do the
'right' thing and the on-disk file case is the right one, even on OSX or
Windows.


> I agree that it's generally nicer to look at, but it causes a bit of a mess to
> normalize to "disk case".

In practice, the on disk case is the same than in the SCM. That's what matters.


> - The rule for how ninja should expect its input is "should be same as disk"?
> That's pretty vague on Windows.

That's vague on OSX too.

> - git does the wrong thing by default on Windows (adding "A" does nothing if
"a"
> already exists).
> - goma will have a hard time caching across machines if any paths (system,
etc)
> are different across machines.
> - It means a disk access (or 2) per normalization, rather than just string
> processing.
> - 'cl /showIncludes' doesn't output in disk case, it's random: excerpt of some
> lines from cl /showIncludes of a file that just does #include <windows.h>
> 
> Note: including file: C:\Program Files (x86)\Microsoft
> SDKs\Windows\v7.0A\include\windows.h
> Note: including file:  c:\Program Files (x86)\Microsoft Visual Studio
> 10.0\VC\INCLUDE\excpt.h
> Note: including file:     c:\program files (x86)\microsoft visual studio
> 10.0\vc\include\codeanalysis\sourceannotations.h
> Note: including file:    c:\program files (x86)\microsoft
> sdks\windows\v7.0a\include\pshpack4.h
> Note: including file:   C:\Program Files (x86)\Microsoft
> SDKs\Windows\v7.0A\include\ime_cmodes.h
> 
> (Different case on drives, different case for VC\INCLUDE, different case for
> program files (x86), etc.)
> 
> Anyway, overall the reason I like lower better is that it's one simple rule
> that's easy to verify, rather than dependent on local state.

[scottmg, 2012-04-27 17:37:22]

Removed case normalization from this CL, just kept the fixes to expansion of VS
vars + relative paths that was rolled in here. Case got complicated, I'll have
to revisit it in a future CL.

[Nico, 2012-04-27 17:41:37]

The code looks ok. Can you update the CL description with a "why" part?

[scottmg, 2012-04-27 17:47:22]

On 2012/04/27 17:41:37, Nico wrote:
> The code looks ok. Can you update the CL description with a "why" part?

Done (realized the VS macro expansion was actually broken, not just
not-normalized)

[Nico, 2012-04-27 23:03:23]

LGTM

CL description still not great (no numbers on how much the perf win is), but not
worth blocking this on.