Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by Michael Achenbach Modified:
4 years, 7 months ago Reviewers:
Nico, jochen (gone - plz use gerrit), Jakob Kummerow 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[build] Prepare moving v8.gyp to src/
This will allow to pull in gyp as a deps to the same location
as chromium (tools/gyp not build/gyp), needed for gn switch.
This is the first step of a 3-way move.
1) Copy v8.gyp in v8
2) Update references in embedders (follow up)
3) Remove old v8.gyp (follow up)
BUG=chromium:474921
LOG=n
NOTRY=true
Committed: https://crrev.com/cb855fe7288f8c440ab12e1aa4f0256e4df9e7e4
Cr-Commit-Position: refs/heads/master@{#35760}
Patch Set 1 #Patch Set 2 : Update references #Patch Set 3 : Delete original gyp file #Patch Set 4 : Fixes #Patch Set 5 : Fixes #Patch Set 6 : Keep old v8.gyp #Patch Set 7 : Better base files. #
Created: 4 years, 8 months ago
Messages
Total messages: 44 (19 generated)
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/patch-status/1920793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920793002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
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/patch-status/1920793002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920793002/60001
Description was changed from ========== [build] Prepare moving v8.gyp BUG= ========== to ========== [build] Prepare moving v8.gyp BUG=chromium:474921 LOG=n ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/4779) v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/656) v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/4801) v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/4783)
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/patch-status/1920793002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920793002/80001
Description was changed from ========== [build] Prepare moving v8.gyp BUG=chromium:474921 LOG=n ========== to ========== [build] Prepare moving v8.gyp This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n ==========
machenbach@chromium.org changed reviewers: + jkummerow@chromium.org, jochen@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/patch-status/1920793002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920793002/100001
PTAL. Patch 5 shows that everything passes with the old v8.gyp removed. Patch 6 keeps it to not break embedders just now.
FYI: I'll also send a PSA around when this lands.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM (). The CL description should probably be updated to "Move v8.gyp to src/".
Description was changed from ========== [build] Prepare moving v8.gyp This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n ========== to ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n ==========
On 2016/04/25 11:21:11, Jakob wrote: > LGTM (). > > The CL description should probably be updated to "Move v8.gyp to src/". I added the "src/" info, but I'm not sure if I should use "Move" as this CL just copies it. The move is comprised of all three CLs of this dance.
can you make sure that it's actually a file move for git (the v8.gyp) one, so it doesn't look like an initial commit?
because now it looks like "A src/v8.gyp" and not "A+ src/v8.gyp"
How about patch 7? It's still not perfect, especially in the beginning of the file, but much better.
lgtm
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/1920793002/#ps120001 (title: "Better base files.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920793002/120001
Description was changed from ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n ========== to ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n NOTRY=true ==========
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920793002/120001
Message was sent while issue was closed.
Description was changed from ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n NOTRY=true ========== to ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n NOTRY=true ========== to ========== [build] Prepare moving v8.gyp to src/ This will allow to pull in gyp as a deps to the same location as chromium (tools/gyp not build/gyp), needed for gn switch. This is the first step of a 3-way move. 1) Copy v8.gyp in v8 2) Update references in embedders (follow up) 3) Remove old v8.gyp (follow up) BUG=chromium:474921 LOG=n NOTRY=true Committed: https://crrev.com/cb855fe7288f8c440ab12e1aa4f0256e4df9e7e4 Cr-Commit-Position: refs/heads/master@{#35760} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/cb855fe7288f8c440ab12e1aa4f0256e4df9e7e4 Cr-Commit-Position: refs/heads/master@{#35760}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
I think this broke the world (silently). https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... e.g. has lots of ninja: warning: multiple rules generate gen/debug-support.cc. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn] (which is only not yet an error due to an accident). It started in that build, and that build has https://codereview.chromium.org/1915963002 which includes this change. This seems potentially very bad, so I'd recommend reverting this if the fix isn't obvious.
Message was sent while issue was closed.
On 2016/04/25 21:05:26, Nico wrote: > I think this broke the world (silently). > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > e.g. has lots of > > ninja: warning: multiple rules generate gen/debug-support.cc. builds involving > this target will not be correct; continuing anyway [-w dupbuild=warn] > > (which is only not yet an error due to an accident). It started in that build, > and that build has https://codereview.chromium.org/1915963002 which includes > this change. > > This seems potentially very bad, so I'd recommend reverting this if the fix > isn't obvious. I tried simply updating Chromium to use the new location of v8.gyp, but that didn't get rid of the warnings.
Message was sent while issue was closed.
On 2016/04/25 23:28:43, adamk wrote: > On 2016/04/25 21:05:26, Nico wrote: > > I think this broke the world (silently). > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > e.g. has lots of > > > > ninja: warning: multiple rules generate gen/debug-support.cc. builds involving > > this target will not be correct; continuing anyway [-w dupbuild=warn] > > > > (which is only not yet an error due to an accident). It started in that build, > > and that build has https://codereview.chromium.org/1915963002 which includes > > this change. > > > > This seems potentially very bad, so I'd recommend reverting this if the fix > > isn't obvious. > > I tried simply updating Chromium to use the new location of v8.gyp, but that > didn't get rid of the warnings. I commented on the bug. "I think the warning is benign because the two gyp files are equal." If all locations from an embedder point to the new v8.gyp, I don't understand why the warning wouldn't go away? Maybe some where missing?
Message was sent while issue was closed.
On 2016/04/25 23:28:43, adamk wrote: > On 2016/04/25 21:05:26, Nico wrote: > > I think this broke the world (silently). > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > e.g. has lots of > > > > ninja: warning: multiple rules generate gen/debug-support.cc. builds involving > > this target will not be correct; continuing anyway [-w dupbuild=warn] > > > > (which is only not yet an error due to an accident). It started in that build, > > and that build has https://codereview.chromium.org/1915963002 which includes > > this change. > > > > This seems potentially very bad, so I'd recommend reverting this if the fix > > isn't obvious. > > I tried simply updating Chromium to use the new location of v8.gyp, but that > didn't get rid of the warnings. It's because of pdfium. If I update the locations in chromium and pdfium, the warnings go away.
Message was sent while issue was closed.
On 2016/04/26 07:25:06, Michael Achenbach wrote: > On 2016/04/25 23:28:43, adamk wrote: > > On 2016/04/25 21:05:26, Nico wrote: > > > I think this broke the world (silently). > > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > > e.g. has lots of > > > > > > ninja: warning: multiple rules generate gen/debug-support.cc. builds > involving > > > this target will not be correct; continuing anyway [-w dupbuild=warn] > > > > > > (which is only not yet an error due to an accident). It started in that > build, > > > and that build has https://codereview.chromium.org/1915963002 which includes > > > this change. > > > > > > This seems potentially very bad, so I'd recommend reverting this if the fix > > > isn't obvious. > > > > I tried simply updating Chromium to use the new location of v8.gyp, but that > > didn't get rid of the warnings. > > I commented on the bug. "I think the warning is benign because the two gyp files > are equal." In practice, the warning is benign only if both build commands for the same file are identical (and only because ninja nowadays is careful to not end up with a broken build graph like it did earlier in these cases). It's up to the generator (gyp) to decide if it wants to be strict about duplicate edges or if it wants to allow duplicate edges if build commands are equal -- gyp just doesn't handle this at all and passes through everything, which means gyp can produce ninja files that are broken. I think the right way to roll this out would've been to change gyp to omit duplicate .o edges if they have the same build command, and then land this. > > If all locations from an embedder point to the new v8.gyp, I don't understand > why the warning wouldn't go away? Maybe some where missing?
Message was sent while issue was closed.
> In practice, the warning is benign only if both build commands for the same file > are identical (and only because ninja nowadays is careful to not end up with a > broken build graph like it did earlier in these cases). It's up to the generator > (gyp) to decide if it wants to be strict about duplicate edges or if it wants to > allow duplicate edges if build commands are equal -- gyp just doesn't handle > this at all and passes through everything, which means gyp can produce ninja > files that are broken. I think the right way to roll this out would've been to > change gyp to omit duplicate .o edges if they have the same build command, and > then land this. > OK. "would've". Now the fixes landed already and we might never do this again ;) But thanks for the explanation! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
