Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1701053002:
gn/win: Try to get chrome_elf_unittests passing on swarming. (Closed)
Descriptiongn/win: Try to get chrome_elf_unittests passing on swarming.
It looks like the indirection in chrome/BUILD.gn that does the
out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to
confuse data_deps, so add an explicit data dep.
Follow-up to https://codereview.chromium.org/1223703002/
BUG=98637, 536192
TBR=caitkp
Committed: https://crrev.com/5cba4757d28e7b73a965c5d23d04c097a6c4d90d
Cr-Commit-Position: refs/heads/master@{#375689}
Patch Set 1 #Patch Set 2 : hmm #
Total comments: 2
Patch Set 3 : hmm #Patch Set 4 : hmmmm #Patch Set 5 : back to 1 #Messages
Total messages: 33 (11 generated)
thakis@chromium.org changed reviewers: + dpranke@chromium.org
I think this is actually the wrong fix; :chrome_elf_unittests should probably have a data_dep on //chrome, and //chrome should have a data_dep on //chrome:chrome_initial. We tripped over this before in crbug.com/536289 , but apparently never actually fixed the dependencies.
On 2016/02/16 18:13:56, Dirk Pranke wrote: > I think this is actually the wrong fix; :chrome_elf_unittests should probably > have a data_dep on //chrome, and //chrome should have a data_dep on > //chrome:chrome_initial. > > We tripped over this before in crbug.com/536289 , but apparently never actually > fixed the dependencies. Actually, looks like we should probably have a data_dep on :reorder_imports as well.
On 2016/02/16 18:15:14, Dirk Pranke wrote: > On 2016/02/16 18:13:56, Dirk Pranke wrote: > > I think this is actually the wrong fix; :chrome_elf_unittests should probably > > have a data_dep on //chrome, and //chrome should have a data_dep on > > //chrome:chrome_initial. > > > > We tripped over this before in crbug.com/536289 , but apparently never > actually > > fixed the dependencies. > > Actually, looks like we should probably have a data_dep on :reorder_imports as > well. There was a data_dep on //chrome; it didn't help. What's "we"? chrome should have a data_dep on :reorder_imports?
Please check if patch set 2 is what you mean.
almost. https://codereview.chromium.org/1701053002/diff/20001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1701053002/diff/20001/chrome/BUILD.gn#newcode68 chrome/BUILD.gn:68: ] I think this needs :reorder_imports as well on win.
Description was changed from ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data line for chrome.exe Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637 ========== to ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data line for chrome.exe Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 ==========
changed more things https://codereview.chromium.org/1701053002/diff/20001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1701053002/diff/20001/chrome/BUILD.gn#newcode68 chrome/BUILD.gn:68: ] On 2016/02/16 19:57:44, Dirk Pranke wrote: > I think this needs :reorder_imports as well on win. Oh, and it should not have :chrome_initial on win, else both chrome.exe and initial/chrome.exe will be bundled up there, right?
Description was changed from ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data line for chrome.exe Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 ========== to ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add more explicit data_deps there. Also remove a previous workaround for the same root cause. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 ==========
On 2016/02/16 20:08:21, Nico wrote: > changed more things > > https://codereview.chromium.org/1701053002/diff/20001/chrome/BUILD.gn > File chrome/BUILD.gn (right): > > https://codereview.chromium.org/1701053002/diff/20001/chrome/BUILD.gn#newcode68 > chrome/BUILD.gn:68: ] > On 2016/02/16 19:57:44, Dirk Pranke wrote: > > I think this needs :reorder_imports as well on win. > > Oh, and it should not have :chrome_initial on win, else both chrome.exe and > initial/chrome.exe will be bundled up there, right? Good point. However, you may still need a data_deps on :chrome_initial (directly or indirectly via :reorder_imports), in order to make sure that all of chrome's data dependencies are propagated. This means you may end up with two copies of chrome.exe, true, but that might not be easily avoidable.
ok, how about i got back to patch set 1 then?
On 2016/02/16 20:20:38, Nico wrote: > ok, how about i got back to patch set 1 then? Does you actually need chrome to be built before you can compile and link chrome_elf_unittests? If so, then I think patchset #1 will work, but I don't particularly like it (we shouldn't be depending on files that aren't part of our target). If you don't actually need chrome to be built (it's just a runtime dependency), then patchset #1 is still wrong. Am I making sense?
doesn't it need to be built if it's a runtime dependency? how can we upload it to swarming if it hasn't been built?
On 2016/02/16 20:24:19, Nico wrote: > doesn't it need to be built if it's a runtime dependency? how can we upload it > to swarming if it hasn't been built? Yes, it needs to be built, but the key part is *before chrome_elf_unittests links*. i.e., if it doesn't, then by putting it in deps (as opposed to data_deps) you're creating a build ordering dependency that doesn't need to exist.
Ah, I see. chrome_elf is a tiny target, so the win from being able to schedule its stuff in parallel to chrome binaries is likely way smaller than the cost to isolate two chrome.exes instead of two. (and the compiles will run in parallel anyways, it's just about serializing the links needlessly, right?)
On 2016/02/16 20:47:10, Nico wrote: > Ah, I see. chrome_elf is a tiny target, so the win from being able to schedule > its stuff in parallel to chrome binaries is likely way smaller than the cost to > isolate two chrome.exes instead of two. (and the compiles will run in parallel > anyways, it's just about serializing the links needlessly, right?) More or less. And conceptual correctness. So, if you want to avoid the two chrome's binaries being in the isolate (which, I agree, is sensible), I'd probably at least add a comment as to why we're adding the dependency on chrome directly when it doesn't really need to be there.
Description was changed from ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add more explicit data_deps there. Also remove a previous workaround for the same root cause. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 ========== to ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 ==========
ok, this is now back at patch set 1 with a comment
lgtm
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701053002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701053002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 ========== to ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 TBR=caitkp ==========
thakis@chromium.org changed reviewers: + caitkp@chromium.org
tbr caitkp for chrome_elf/OWNERS again
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701053002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701053002/80001
Message was sent while issue was closed.
Description was changed from ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 TBR=caitkp ========== to ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 TBR=caitkp ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 TBR=caitkp ========== to ========== gn/win: Try to get chrome_elf_unittests passing on swarming. It looks like the indirection in chrome/BUILD.gn that does the out/foo/initial/chrome.exe -> out/foo/chrome.exe mananges to confuse data_deps, so add an explicit data dep. Follow-up to https://codereview.chromium.org/1223703002/ BUG=98637,536192 TBR=caitkp Committed: https://crrev.com/5cba4757d28e7b73a965c5d23d04c097a6c4d90d Cr-Commit-Position: refs/heads/master@{#375689} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5cba4757d28e7b73a965c5d23d04c097a6c4d90d Cr-Commit-Position: refs/heads/master@{#375689} |
