Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2109083002:
[gn] Switch linux64 release to gn. (Closed)
|
Created:
4 years, 5 months ago by Michael Achenbach Modified:
4 years, 5 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gn] Switch linux64 release to gn.
BUG=chromium:474921
Committed: https://crrev.com/b4b45ff6927897bead251c6f2a3421824a043089
Cr-Commit-Position: refs/heads/master@{#37385}
Patch Set 1 #
Messages
Total messages: 15 (7 generated)
Description was changed from ========== [gn] Switch linux64 release to gn. BUG= ========== to ========== [gn] Switch linux64 release to gn. BUG=chromium:474921 ==========
machenbach@chromium.org changed reviewers: + jochen@chromium.org, vogelheim@chromium.org
The CQ bit was checked by machenbach@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...
PTAL. I think these bots are ready to be switched. Links to the flag comparison (please skim through these): https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20builder/bu... https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/8... Some things that caught my attention, but might not be relevant: - ICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE is set for a few files only in gn e.g. d8.cc, hello-world.cc, ... I think it is correct that it's set now and it's not worth fixing gyp. I assume it's also not relevant to these files, otherwise we'd see some failures already. - GN doesn't work well with out directories outside the project root. The fyi step on the bots uses a tmp dir for the gn preview output. This leads to paths being joined wrongly. This won't matter, as when switching to gn, the gn step uses the normal out dir in the project tree. - The -O3 option included v8 and thirdparty sources in gyp. Now only v8. But that's as we ship anyways.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm I was surprised at -DV8_DEPRECATION_WARNINGS in GN. That should be an improvement, though? Also, surprised at -DCHROMIUM_BUILD. We don't use that symbol, so we presumably don't care, but I take it that's meant to distinguish Chromium builds, and these are v8-only builds, so that usage is wrong?
On 2016/06/29 09:43:08, vogelheim wrote: > lgtm > > I was surprised at -DV8_DEPRECATION_WARNINGS in GN. That should be an > improvement, though? This affects only thirdparty code, icu and gtest/gmock. We have this pattern in other cases, that thirdparty code doesn't get the same configs applied. In this case, it's good I think, as those components don't understand the flag anyways. > Also, surprised at -DCHROMIUM_BUILD. We don't use that symbol, so we presumably > don't care, but I take it that's meant to distinguish Chromium builds, and these > are v8-only builds, so that usage is wrong? Also here, v8 doesn't understand this flag, so it shouldn't matter. This flag and many others are set on a build-global level defined in chromium. It is lower prio to make that nicer for us.
The CQ bit was checked by machenbach@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.
Description was changed from ========== [gn] Switch linux64 release to gn. BUG=chromium:474921 ========== to ========== [gn] Switch linux64 release to gn. BUG=chromium:474921 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [gn] Switch linux64 release to gn. BUG=chromium:474921 ========== to ========== [gn] Switch linux64 release to gn. BUG=chromium:474921 Committed: https://crrev.com/b4b45ff6927897bead251c6f2a3421824a043089 Cr-Commit-Position: refs/heads/master@{#37385} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/b4b45ff6927897bead251c6f2a3421824a043089 Cr-Commit-Position: refs/heads/master@{#37385} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
