Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2621983002:
Pull define for version out into v8-version-string.h and separate build target (Closed)
DescriptionPull define for version out into v8-version-string.h and separate build target
This is part of removing the dependency of the Chromium browser DLL on
Windows on V8.
R=jochen@chromium.org
BUG=chromium:581766
Review-Url: https://codereview.chromium.org/2621983002
Cr-Original-Commit-Position: refs/heads/master@{#42243}
Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c3ba6d
Review-Url: https://codereview.chromium.org/2621983002
Cr-Commit-Position: refs/heads/master@{#42289}
Committed: https://chromium.googlesource.com/v8/v8/+/ffc0931f879b5aeb48490d1ff34f710b45462e11
Patch Set 1 #Patch Set 2 : v8-version-string.h #
Total comments: 2
Patch Set 3 : forgotten removal #
Messages
Total messages: 29 (11 generated)
jochen@chromium.org changed reviewers: + machenbach@chromium.org
deferring to machenbach@ as I don't know whether we have any scripts that might not like the change
LGTM with remark: We usually also keep gyp in sync for node.js, but it's probably ok to not have this particular target there. This shouldn't break the infrastructure. A summary of the tools can be found here: https://bugs.chromium.org/p/v8/issues/detail?id=5740 The tools, including omahaproxy, only read the MAJOR, MINOR, BUILD, PATCH and IS_CANDIDATE_VERSION defines. In the bug there is also a CL in flight that likely collides a bit with this one, but probably easy to resolve.
On 2017/01/11 10:05:36, Michael Achenbach wrote: > LGTM with remark: We usually also keep gyp in sync for node.js, but it's > probably ok to not have this particular target there. > > This shouldn't break the infrastructure. A summary of the tools can be found > here: > https://bugs.chromium.org/p/v8/issues/detail?id=5740 > > The tools, including omahaproxy, only read the MAJOR, MINOR, BUILD, PATCH and > IS_CANDIDATE_VERSION defines. > > In the bug there is also a CL in flight that likely collides a bit with this > one, but probably easy to resolve. OK. I'll try CQ'ing as-is then. I'm happy to do any followups for gyp or resolving conflicts with the other work. LMK.
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1484156919938040, "parent_rev":
"a2efde46a1678e67ad20fc5ccda471beb5e24a4d", "commit_rev":
"45938454177f53fa24cfc08ad97ccbc162c3ba6d"}
Message was sent while issue was closed.
Description was changed from ========== Pull define for version out into v8-version.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 ========== to ========== Pull define for version out into v8-version.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2627713008/ by hablich@chromium.org. The reason for reverting is: Blocks roll: https://codereview.chromium.org/2633463002/.
Message was sent while issue was closed.
Description was changed from ========== Pull define for version out into v8-version.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... ========== to ========== Pull define for version out into v8-version.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... ==========
Message was sent while issue was closed.
scottmg@chromium.org changed reviewers: + hablich@chromium.org
Message was sent while issue was closed.
On 2017/01/12 09:26:19, Michael Hablich wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.chromium.org/2627713008/ by mailto:hablich@chromium.org. > > The reason for reverting is: Blocks roll: > https://codereview.chromium.org/2633463002/. Thanks for reverting. The error is: ../../v8/src/version.cc:24:40: error: use of undeclared identifier 'V8_VERSION_STRING' const char* Version::version_string_ = V8_VERSION_STRING; Very strange, I'm not sure what's wrong. - A local build is fine (I made the change in Chromium and then had to figure out how to land upstream here) - By inspection it looks like include/v8-version.h does happily define that, and it's included by version.cc. So I guess it must be that the recipe https://cs.chromium.org/chromium/build/scripts/slave/recipes/v8/auto_tag.py?q... is modifying v8-version.h and losing that define somehow? But looking at what it does, I don't see how that's happening. I can try and figure out what's going on, but it seems there's a bit more automated parsing of v8-version.h than I realized, so maybe it's better to add a new separate header instead anyway (something like v8-version-string.h) that does the string derivation to leave v8-version.h alone and be more easily machine readable.
Message was sent while issue was closed.
On 2017/01/12 17:35:28, scottmg wrote: > On 2017/01/12 09:26:19, Michael Hablich wrote: > > A revert of this CL (patchset #1 id:1) has been created in > > https://codereview.chromium.org/2627713008/ by mailto:hablich@chromium.org. > > > > The reason for reverting is: Blocks roll: > > https://codereview.chromium.org/2633463002/. > > Thanks for reverting. > > The error is: > > ../../v8/src/version.cc:24:40: error: use of undeclared identifier > 'V8_VERSION_STRING' > const char* Version::version_string_ = V8_VERSION_STRING; > > Very strange, I'm not sure what's wrong. > - A local build is fine (I made the change in Chromium and then had to figure > out how to land upstream here) > - By inspection it looks like include/v8-version.h does happily define that, and > it's included by version.cc. > > So I guess it must be that the recipe > https://cs.chromium.org/chromium/build/scripts/slave/recipes/v8/auto_tag.py?q... > is modifying v8-version.h and losing that define somehow? But looking at what it > does, I don't see how that's happening. (Or perhaps https://cs.chromium.org/chromium/src/v8/tools/release/common_includes.py?q=v8... though again, it sort of looks fine.) > > I can try and figure out what's going on, but it seems there's a bit more > automated parsing of v8-version.h than I realized, so maybe it's better to add a > new separate header instead anyway (something like v8-version-string.h) that > does the string derivation to leave v8-version.h alone and be more easily > machine readable.
Description was changed from ========== Pull define for version out into v8-version.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... ========== to ========== Pull define for version out into v8-version-string.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... ==========
Please take another look.
On 2017/01/12 18:39:47, scottmg wrote: > Please take another look.
lgtm if chromium trybots are happy... https://codereview.chromium.org/2621983002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2621983002/diff/20001/BUILD.gn#newcode878 BUILD.gn:878: "include/v8-version.h", You don't remove it here anymore as in patch 1?
Thanks for looking so late. https://codereview.chromium.org/2621983002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2621983002/diff/20001/BUILD.gn#newcode878 BUILD.gn:878: "include/v8-version.h", On 2017/01/12 19:42:55, Michael Achenbach wrote: > You don't remove it here anymore as in patch 1? Oops, done.
On 2017/01/12 19:42:55, Michael Achenbach wrote: > lgtm if chromium trybots are happy... A selection (cros, win, mac, linux) have happily compiled version.cc. > > https://codereview.chromium.org/2621983002/diff/20001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/2621983002/diff/20001/BUILD.gn#newcode878 > BUILD.gn:878: "include/v8-version.h", > You don't remove it here anymore as in patch 1?
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2621983002/#ps40001 (title: "forgotten removal")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484252727483140,
"parent_rev": "022635bf0ddb22f26654621a86d4c81430114403", "commit_rev":
"ffc0931f879b5aeb48490d1ff34f710b45462e11"}
Message was sent while issue was closed.
Description was changed from ========== Pull define for version out into v8-version-string.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... ========== to ========== Pull define for version out into v8-version-string.h and separate build target This is part of removing the dependency of the Chromium browser DLL on Windows on V8. R=jochen@chromium.org BUG=chromium:581766 Review-Url: https://codereview.chromium.org/2621983002 Cr-Original-Commit-Position: refs/heads/master@{#42243} Committed: https://chromium.googlesource.com/v8/v8/+/45938454177f53fa24cfc08ad97ccbc162c... Review-Url: https://codereview.chromium.org/2621983002 Cr-Commit-Position: refs/heads/master@{#42289} Committed: https://chromium.googlesource.com/v8/v8/+/ffc0931f879b5aeb48490d1ff34f710b454... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/ffc0931f879b5aeb48490d1ff34f710b454...
Message was sent while issue was closed.
Sigh, I guess this will roll OK, but isn't useful because include/v8-version-string.h does #include "include/v8-version.h" which is OK from src/version.cc, but not from <chromium>/src/chrome/something/or/other which I guess doesn't have <chromium>/src/v8/ in the include search path. :(
Message was sent while issue was closed.
On 2017/01/12 21:44:10, scottmg wrote: > Sigh, I guess this will roll OK, but isn't useful because > include/v8-version-string.h does > > #include "include/v8-version.h" > > which is OK from src/version.cc, but not from > <chromium>/src/chrome/something/or/other which I guess doesn't have > <chromium>/src/v8/ in the include search path. :( Followup fix here: https://codereview.chromium.org/2634443002. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
