Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237233002:
Clean up GN template code. (Closed)
|
Created:
4 years, 4 months ago by brettw Modified:
4 years, 4 months ago CC:
chromium-reviews, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, dmazzoni+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up GN template code.
This cleans up some variable definitions around templates not marking variables used.
The chromevox directory worked around the bug by putting the variables to share
in a .gni file. This file can now be deleted and the contents merged with the
BUILD file (the only place it was included).
The iOS code referencing this bug seems to be mistaken, at least in some of the
cases. The conditions in that file do not trigger the variable used bug. The
build seems to work without this code, so I removed it.
BUG=395883
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/84f93c430207e806a6a3746d4163990391c1fbc1
Cr-Commit-Position: refs/heads/master@{#411738}
Patch Set 1 #
Total comments: 1
Patch Set 2 : no ios #Patch Set 3 : chromevox #
Messages
Total messages: 35 (21 generated)
Description was changed from ========== Clean up GN template code. This cleans up some variable definitions around templates not marking variables used. The chromevox directory worked around the bug by putting the variables to share in a .gni file. This file can now be deleted and the contents merged with the BUILD file (the only place it was included). The iOS code referencing this bug seems to be mistaken, at least in some of the cases. The conditions in that file do not trigger the variable used bug. The build seems to work without this code, so I removed it. BUG=395883 ========== to ========== Clean up GN template code. This cleans up some variable definitions around templates not marking variables used. The chromevox directory worked around the bug by putting the variables to share in a .gni file. This file can now be deleted and the contents merged with the BUILD file (the only place it was included). The iOS code referencing this bug seems to be mistaken, at least in some of the cases. The conditions in that file do not trigger the variable used bug. The build seems to work without this code, so I removed it. BUG=395883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
brettw@chromium.org changed reviewers: + sdefresne@chromium.org
Note: Requires https://codereview.chromium.org/2235373002/ to land first.
I won't be able to look at this for a while due to travelling (imminent). You may want to ask someone else to review this CL if you are looking for a review in less than 24 hours. https://codereview.chromium.org/2237233002/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (left): https://codereview.chromium.org/2237233002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:503: # TODO(crbug.com/395883): ensure those variables are marked as used to Those are removed as part of https://codereview.chromium.org/2219323002/. I would recommend dropping the changes in that file to avoid conflicting.
On 2016/08/11 23:23:03, sdefresne (travelling - slow) wrote: > I won't be able to look at this for a while due to travelling (imminent). You > may want to ask someone else to review this CL if you are looking for a review > in less than 24 hours. > > https://codereview.chromium.org/2237233002/diff/1/build/config/ios/rules.gni > File build/config/ios/rules.gni (left): > > https://codereview.chromium.org/2237233002/diff/1/build/config/ios/rules.gni#... > build/config/ios/rules.gni:503: # TODO(crbug.com/395883): ensure those variables > are marked as used to > Those are removed as part of https://codereview.chromium.org/2219323002/. I > would recommend dropping the changes in that file to avoid conflicting. Oh great, that was the whole reason I added you in the first place.
brettw@chromium.org changed reviewers: + dpranke@chromium.org - sdefresne@chromium.org
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chromevox
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
brettw@chromium.org changed reviewers: + scottmg@chromium.org - dpranke@chromium.org
-> Scott
lgtm
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Clean up GN template code. This cleans up some variable definitions around templates not marking variables used. The chromevox directory worked around the bug by putting the variables to share in a .gni file. This file can now be deleted and the contents merged with the BUILD file (the only place it was included). The iOS code referencing this bug seems to be mistaken, at least in some of the cases. The conditions in that file do not trigger the variable used bug. The build seems to work without this code, so I removed it. BUG=395883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Clean up GN template code. This cleans up some variable definitions around templates not marking variables used. The chromevox directory worked around the bug by putting the variables to share in a .gni file. This file can now be deleted and the contents merged with the BUILD file (the only place it was included). The iOS code referencing this bug seems to be mistaken, at least in some of the cases. The conditions in that file do not trigger the variable used bug. The build seems to work without this code, so I removed it. BUG=395883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/84f93c430207e806a6a3746d4163990391c1fbc1 Cr-Commit-Position: refs/heads/master@{#411738} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/84f93c430207e806a6a3746d4163990391c1fbc1 Cr-Commit-Position: refs/heads/master@{#411738}
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
