Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
321813002:
Remove an obsolete comment (Closed)
|
Created:
6 years, 6 months ago by yukawa Modified:
6 years, 6 months ago Reviewers:
scottmg CC:
gyp-developer_googlegroups.com, Nico Base URL:
http://gyp.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionRemove an obsolete comment
This CL changes nothing but removes a comment added in r1620 as it should be obsolete thanks to Visual Studio 2013 transition in Chromium.
Note that 'cc' and 'cxx' were defined as 'UNKNOWN' before r1620, but this CL leaves them to be 'cl.exe' because some tests have already relied on these default values.
BUG=chromium:233985
TEST=unittest
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comment #Patch Set 3 : Abondone changes in the patch set #2 due to test failures #Messages
Total messages: 9 (0 generated)
Hi Scott, Can we remove this comment? I'd like to add some lines around here in another CL for BUG=gyp:434.
lgtm (I'm not sure how the cl_x86 stuff will interact with your make_global_settings goal exactly yet) https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (left): https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' Yes, this can be removed now. Can you also change cc = 'UNSET' cxx = 'UNSET' so that if there's something that's not setting cc via cc = cl_x86 or cl_x64 we'll see an obvious failure?
https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (left): https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' On 2014/06/09 18:02:09, scottmg wrote: > Yes, this can be removed now. Can you also change > > cc = 'UNSET' > cxx = 'UNSET' > > so that if there's something that's not setting cc via cc = cl_x86 or cl_x64 > we'll see an obvious failure? Thanks. Done. Also revised the CL description accordingly.
On 2014/06/10 02:46:26, yukawa wrote: > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py > File pylib/gyp/generator/ninja.py (left): > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... > pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' > On 2014/06/09 18:02:09, scottmg wrote: > > Yes, this can be removed now. Can you also change > > > > cc = 'UNSET' > > cxx = 'UNSET' > > > > so that if there's something that's not setting cc via cc = cl_x86 or cl_x64 > > we'll see an obvious failure? > > Thanks. Done. Also revised the CL description accordingly. Hi Scott, Test failures in try jobs for the patch set #2 seem to be real. So I've partially reverted changes in the patch set #2. I still think we can remove the comment itself, but I'm wondering if it's a bit too late to unset the default value of cc/cxx for Ninja generator on Windows. What do you think?
On 2014/06/10 15:12:05, yukawa wrote: > On 2014/06/10 02:46:26, yukawa wrote: > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py > > File pylib/gyp/generator/ninja.py (left): > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... > > pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' > > On 2014/06/09 18:02:09, scottmg wrote: > > > Yes, this can be removed now. Can you also change > > > > > > cc = 'UNSET' > > > cxx = 'UNSET' > > > > > > so that if there's something that's not setting cc via cc = cl_x86 or cl_x64 > > > we'll see an obvious failure? > > > > Thanks. Done. Also revised the CL description accordingly. > > Hi Scott, > Test failures in try jobs for the patch set #2 seem to be real. So I've > partially reverted changes in the patch set #2. > > I still think we can remove the comment itself, but I'm wondering if it's a bit > too late to unset the default value of cc/cxx for Ninja generator on Windows. > What do you think? Note that the failed test in the patch set #2 is test/cflags/cflags.gyp, which has been enabled not only for Linux but also for Win and Mac since https://code.google.com/p/gyp/source/detail?r=1699.
On 2014/06/10 15:21:54, yukawa wrote: > On 2014/06/10 15:12:05, yukawa wrote: > > On 2014/06/10 02:46:26, yukawa wrote: > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py > > > File pylib/gyp/generator/ninja.py (left): > > > > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... > > > pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' > > > On 2014/06/09 18:02:09, scottmg wrote: > > > > Yes, this can be removed now. Can you also change > > > > > > > > cc = 'UNSET' > > > > cxx = 'UNSET' > > > > > > > > so that if there's something that's not setting cc via cc = cl_x86 or > cl_x64 > > > > we'll see an obvious failure? > > > > > > Thanks. Done. Also revised the CL description accordingly. > > > > Hi Scott, > > Test failures in try jobs for the patch set #2 seem to be real. So I've > > partially reverted changes in the patch set #2. > > > > I still think we can remove the comment itself, but I'm wondering if it's a > bit > > too late to unset the default value of cc/cxx for Ninja generator on Windows. > > What do you think? > > Note that the failed test in the patch set #2 is test/cflags/cflags.gyp, which > has been enabled not only for Linux but also for Win and Mac since > https://code.google.com/p/gyp/source/detail?r=1699. (Sorry, was out yesterday) I don't see why this is failing as I wouldn't expect overriding CFLAGS to affect the compiler chosen, and I think we shouldn't be relying on it being set to the default as it's not clear what architecture is being built then. I assume this isn't blocking you since it's just the comment now. I will take a look at it today.
On 2014/06/11 13:38:28, scottmg wrote: > On 2014/06/10 15:21:54, yukawa wrote: > > On 2014/06/10 15:12:05, yukawa wrote: > > > On 2014/06/10 02:46:26, yukawa wrote: > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py > > > > File pylib/gyp/generator/ninja.py (left): > > > > > > > > > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... > > > > pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' > > > > On 2014/06/09 18:02:09, scottmg wrote: > > > > > Yes, this can be removed now. Can you also change > > > > > > > > > > cc = 'UNSET' > > > > > cxx = 'UNSET' > > > > > > > > > > so that if there's something that's not setting cc via cc = cl_x86 or > > cl_x64 > > > > > we'll see an obvious failure? > > > > > > > > Thanks. Done. Also revised the CL description accordingly. > > > > > > Hi Scott, > > > Test failures in try jobs for the patch set #2 seem to be real. So I've > > > partially reverted changes in the patch set #2. > > > > > > I still think we can remove the comment itself, but I'm wondering if it's a > > bit > > > too late to unset the default value of cc/cxx for Ninja generator on > Windows. > > > What do you think? > > > > Note that the failed test in the patch set #2 is test/cflags/cflags.gyp, which > > has been enabled not only for Linux but also for Win and Mac since > > https://code.google.com/p/gyp/source/detail?r=1699. > > (Sorry, was out yesterday) > > I don't see why this is failing as I wouldn't expect overriding CFLAGS to affect > the compiler chosen, and I think we shouldn't be relying on it being set to the > default as it's not clear what architecture is being built then. > > I assume this isn't blocking you since it's just the comment now. I will take a > look at it today. I posted a CL here that is ps#2 + setting cc_host/cxx_host, which I believe was only working accidentally before.
On 2014/06/11 16:37:17, scottmg wrote: > On 2014/06/11 13:38:28, scottmg wrote: > > On 2014/06/10 15:21:54, yukawa wrote: > > > On 2014/06/10 15:12:05, yukawa wrote: > > > > On 2014/06/10 02:46:26, yukawa wrote: > > > > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py > > > > > File pylib/gyp/generator/ninja.py (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... > > > > > pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' > > > > > On 2014/06/09 18:02:09, scottmg wrote: > > > > > > Yes, this can be removed now. Can you also change > > > > > > > > > > > > cc = 'UNSET' > > > > > > cxx = 'UNSET' > > > > > > > > > > > > so that if there's something that's not setting cc via cc = cl_x86 or > > > cl_x64 > > > > > > we'll see an obvious failure? > > > > > > > > > > Thanks. Done. Also revised the CL description accordingly. > > > > > > > > Hi Scott, > > > > Test failures in try jobs for the patch set #2 seem to be real. So I've > > > > partially reverted changes in the patch set #2. > > > > > > > > I still think we can remove the comment itself, but I'm wondering if it's > a > > > bit > > > > too late to unset the default value of cc/cxx for Ninja generator on > > Windows. > > > > What do you think? > > > > > > Note that the failed test in the patch set #2 is test/cflags/cflags.gyp, > which > > > has been enabled not only for Linux but also for Win and Mac since > > > https://code.google.com/p/gyp/source/detail?r=1699. > > > > (Sorry, was out yesterday) > > > > I don't see why this is failing as I wouldn't expect overriding CFLAGS to > affect > > the compiler chosen, and I think we shouldn't be relying on it being set to > the > > default as it's not clear what architecture is being built then. > > > > I assume this isn't blocking you since it's just the comment now. I will take > a > > look at it today. > > I posted a CL here that is ps#2 + setting cc_host/cxx_host, which I believe was > only > working accidentally before. "Here" even: https://codereview.chromium.org/324383004
On 2014/06/11 16:37:27, scottmg wrote: > On 2014/06/11 16:37:17, scottmg wrote: > > On 2014/06/11 13:38:28, scottmg wrote: > > > On 2014/06/10 15:21:54, yukawa wrote: > > > > On 2014/06/10 15:12:05, yukawa wrote: > > > > > On 2014/06/10 02:46:26, yukawa wrote: > > > > > > > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py > > > > > > File pylib/gyp/generator/ninja.py (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/321813002/diff/1/pylib/gyp/generator/ninja.py... > > > > > > pylib/gyp/generator/ninja.py:1721: cc = 'cl.exe' > > > > > > On 2014/06/09 18:02:09, scottmg wrote: > > > > > > > Yes, this can be removed now. Can you also change > > > > > > > > > > > > > > cc = 'UNSET' > > > > > > > cxx = 'UNSET' > > > > > > > > > > > > > > so that if there's something that's not setting cc via cc = cl_x86 > or > > > > cl_x64 > > > > > > > we'll see an obvious failure? > > > > > > > > > > > > Thanks. Done. Also revised the CL description accordingly. > > > > > > > > > > Hi Scott, > > > > > Test failures in try jobs for the patch set #2 seem to be real. So I've > > > > > partially reverted changes in the patch set #2. > > > > > > > > > > I still think we can remove the comment itself, but I'm wondering if > it's > > a > > > > bit > > > > > too late to unset the default value of cc/cxx for Ninja generator on > > > Windows. > > > > > What do you think? > > > > > > > > Note that the failed test in the patch set #2 is test/cflags/cflags.gyp, > > which > > > > has been enabled not only for Linux but also for Win and Mac since > > > > https://code.google.com/p/gyp/source/detail?r=1699. > > > > > > (Sorry, was out yesterday) > > > > > > I don't see why this is failing as I wouldn't expect overriding CFLAGS to > > affect > > > the compiler chosen, and I think we shouldn't be relying on it being set to > > the > > > default as it's not clear what architecture is being built then. > > > > > > I assume this isn't blocking you since it's just the comment now. I will > take > > a > > > look at it today. > > > > I posted a CL here that is ps#2 + setting cc_host/cxx_host, which I believe > was > > only > > working accidentally before. > > "Here" even: https://codereview.chromium.org/324383004 Thank you so much! I'll close this CL after https://codereview.chromium.org/324383004 is committed. Can you commit your CL? |
