Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1188553003:
MSVS: Normalize paths against gyp directory. (Closed)
|
Created:
5 years, 6 months ago by Jamie Madill Modified:
5 years, 6 months ago CC:
gyp-developer_googlegroups.com Base URL:
http://chromium.googlesource.com/external/gyp@master Target Ref:
refs/heads/master Project:
gyp Visibility:
Public. |
DescriptionMSVS: Normalize paths against gyp directory.
This trims down some redundant paths such as ../../src/code/File.cpp
if the gyp file is also in src/code.
Patch from jmadill@chromium.org.
BUG=None
R=sbaig1@bloomberg.net, scottmg@chromium.org
Committed: https://chromium.googlesource.com/external/gyp/+/b4781fc38236b0fb1238969c918a75a200cfffdb
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address scottmg #Patch Set 3 : With git add of new file #
Total comments: 2
Patch Set 4 : Fix last style change #
Messages
Total messages: 22 (4 generated)
jmadill@chromium.org changed reviewers: + scottmg@chromium.org
PTAL Scott, or send it to the appropriate MSVS guru for review. This change cleans up ANGLE's paths in our generated MSVS projects, and should be fairly inconsequential for most projects.
scottmg@chromium.org changed reviewers: + sbaig1@bloomberg.net
Can we add a test for this? +shez might know/care more about the msvs generator. https://codereview.chromium.org/1188553003/diff/1/pylib/gyp/generator/msvs.py File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1188553003/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:137: def _NormalizedSource(source, curdir = None): here too https://codereview.chromium.org/1188553003/diff/1/pylib/gyp/generator/msvs.py... pylib/gyp/generator/msvs.py:177: def _FixPaths(paths, curdir = None): nit; No spaces around = for default args.
On 2015/06/15 18:46:52, scottmg wrote: > Can we add a test for this? > > +shez might know/care more about the msvs generator. > > https://codereview.chromium.org/1188553003/diff/1/pylib/gyp/generator/msvs.py > File pylib/gyp/generator/msvs.py (right): > > https://codereview.chromium.org/1188553003/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:137: def _NormalizedSource(source, curdir = None): > here too > > https://codereview.chromium.org/1188553003/diff/1/pylib/gyp/generator/msvs.py... > pylib/gyp/generator/msvs.py:177: def _FixPaths(paths, curdir = None): > nit; No spaces around = for default args. Added test, fixed nits. PTAL.
lgtm If I understand correctly, this cleans up the project file when a gyp file contains references to sources like "../subdir/foo.cc" and the gypfile itself happens to be in the subdir? Out of curiosity, why wouldn't the gypfile just use "foo.cc" in that case?
Shezan, that's a sensible question which sadly has a convoluted answer. ANGLE's tests use gypi files which we share between the Chromium and standalone ANGLE integrations, as well as between GN and Gyp. Because GN pulls in lists of sources from gyp variables, we store our sources as variables an then expand them in the angle gyp sources fields. For whatever reason, gyp isn't happy when we use gypi files from a different directory than the current gyp file, and the sources use relative paths. I'd be happy to show you if you want more detail, but this patch does clean up our projects.
On 2015/06/15 22:28:53, Jamie Madill wrote: > Shezan, that's a sensible question which sadly has a convoluted answer. ANGLE's > tests use gypi files which we share between the Chromium and standalone ANGLE > integrations, as well as between GN and Gyp. Because GN pulls in lists of > sources from gyp variables, we store our sources as variables an then expand > them in the angle gyp sources fields. For whatever reason, gyp isn't happy when > we use gypi files from a different directory than the current gyp file, and the > sources use relative paths. I'd be happy to show you if you want more detail, > but this patch does clean up our projects. Thanks for the explanation :) Still lgtm
The CQ bit was checked by jmadill@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
lgtm, I can land https://codereview.chromium.org/1188553003/diff/40001/pylib/gyp/generator/msv... File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1188553003/diff/40001/pylib/gyp/generator/msv... pylib/gyp/generator/msvs.py:159: def _FixPath(path, curdir = None): no spaces here (sorry, I know it's annoying)
PTAL https://codereview.chromium.org/1188553003/diff/40001/pylib/gyp/generator/msv... File pylib/gyp/generator/msvs.py (right): https://codereview.chromium.org/1188553003/diff/40001/pylib/gyp/generator/msv... pylib/gyp/generator/msvs.py:159: def _FixPath(path, curdir = None): On 2015/06/15 23:04:00, scottmg wrote: > no spaces here (sorry, I know it's annoying) Done.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as b4781fc38236b0fb1238969c918a75a200cfffdb.
Message was sent while issue was closed.
This might have broken the v8 XP bots (works on our Win7 bots): http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%201/builds/2849... Can we do something on the v8-side to make this work?
Message was sent while issue was closed.
On 2015/06/17 11:25:45, Michael Achenbach wrote: > This might have broken the v8 XP bots (works on our Win7 bots): > http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%201/builds/2849... > > Can we do something on the v8-side to make this work? Hmm, looks like msvs wants the rule inputs to match *exactly* the source files. You should be able to workaround this in v8.gyp by replacing references to "../../tools/js2c.py" with "../js2c.py", but I think it is better to fix this in gyp itself. Jamie, can you upload a new patch that normalizes rule/action inputs/outputs as well?
Message was sent while issue was closed.
On 2015/06/17 11:25:45, Michael Achenbach wrote: > This might have broken the v8 XP bots (works on our Win7 bots): > http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%201/builds/2849... > Not sure why it works in Win7 but not in XP. Are they using different versions of msvs?
Message was sent while issue was closed.
On 2015/06/17 13:19:51, Shezan Baig (Bloomberg) wrote: > On 2015/06/17 11:25:45, Michael Achenbach wrote: > > This might have broken the v8 XP bots (works on our Win7 bots): > > > http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%201/builds/2849... > > > > Can we do something on the v8-side to make this work? > > > Hmm, looks like msvs wants the rule inputs to match *exactly* the source files. > You should be able to workaround this in v8.gyp by replacing references to > "../../tools/js2c.py" with "../js2c.py", but I think it is better to fix this in > gyp itself. Jamie, can you upload a new patch that normalizes rule/action > inputs/outputs as well? Ok, I read the error message wrong. This isn't an error when msvs builds the project, but the error is at gyp time. And from the stack, it looks like it is generating the old-style msvs projects (for VS2008?), which is a pretty old generator :) Nevertheless, a gyp patch to normalize the rule/action inputs/outputs should fix this, I think. Note that the gyp test bots (once they come up) only run tests for VS2013 (maybe VS2015 as well?), so anything older than that is only soft-supported.
Message was sent while issue was closed.
Sure, I'll give it a go.
Message was sent while issue was closed.
On 2015/06/17 15:24:23, Jamie Madill wrote: > Sure, I'll give it a go. Thanks! Please ping me when this is ready. Our auto-deps roller will reland the gyp change once per day and the XP bots don't have CQ coverage. If it takes a while, I can try to disable it temporarily. The MSVS version on these bots is old. They should probably not run hooks at all as they are pure testers, unfortunately some hooks are build- some are test-specific.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1193573002/ by jmadill@chromium.org. The reason for reverting is: Breaking VS2008 support for V8..
Message was sent while issue was closed.
Thanks. A revert also does the trick. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
