Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469353002:
Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. (Closed)
|
Created:
4 years, 1 month ago by Łukasz Anforowicz Modified:
4 years, 1 month ago CC:
chromium-reviews, jam, darin-cc_chromium.org, nasko, site-isolation-reviews_chromium.org, slan, servolk, Paweł Hajdan Jr. Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert Site Isolation Win bot to --isolate-extensions [2nd attempt].
This is an attempt to reland https://crrev.com/2459813003 after:
1) proper merging (i.e. moving the diff hunks further down so they
apply to Site Isolation Win config, not to Site Isolation Linux
as they've accidentally been merged by CQ in the earlier CL)
2) adding data dependencies for the filter files
(with extra refactoring / consolidation to hopefully make things
easier / less surprising in the future).
BUG=545200
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/e8b43a499acd3df1ff12f08f2652d4acca2695fd
Cr-Commit-Position: refs/heads/master@{#430603}
Patch Set 1 #Patch Set 2 : Fixing a silly mistake from the previous patchset - using absolute paths as needed. #
Total comments: 4
Patch Set 3 : Added reference to bug 658853 in the comments. #
Total comments: 4
Patch Set 4 : Introducing testing/buildbot/filters/BUILD.gn #Patch Set 5 : Rebasing to minimize chance of merge conflicts (and of another https://crbug.com/661447). #
Total comments: 1
Messages
Total messages: 50 (30 generated)
The CQ bit was checked by lukasza@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...
Description was changed from ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after' 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@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...
lukasza@chromium.org changed reviewers: + alexmos@chromium.org
alexmos@, can you take a look please?
LGTM, and thanks for also cleaning up BUILD.gn to add all filter files as dependencies. I think that's a good step to prevent issues like 661447 from happening again as bot configs get changed around. We could even go one step further and add a README to testing/buildbot/filters/, mentioning that new filter files should also be added as build dependencies for swarming bots. https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn#n... content/test/BUILD.gn:733: # corresponding dependency changes here). nit: might be helpful to also add a reference to 661447 (in both comments) https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn#n... content/test/BUILD.gn:741: "//testing/buildbot/filters/cast-linux.content_browsertests.filter", I wonder how the Cast Linux bot manages to work, since it seems to use this filter file as well as swarming, but before this, the filter file wasn't included as a dependency.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review. On 2016/11/03 01:12:15, alexmos wrote: > LGTM, and thanks for also cleaning up BUILD.gn to add all filter files as > dependencies. I think that's a good step to prevent issues like 661447 from > happening again as bot configs get changed around. We could even go one step > further and add a README to testing/buildbot/filters/, mentioning that new > filter files should also be added as build dependencies for swarming bots. Good ideas. I've tried doing this in https://crrev.com/2472153002 (and given how much information I've managed to put into the first version of this file, it seems like something we should have done a while ago :-). Also - writing down the README.md file, made me realize a problem in an Android implementation - https://crrev.com/2477813002 https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn#n... content/test/BUILD.gn:733: # corresponding dependency changes here). On 2016/11/03 01:12:14, alexmos wrote: > nit: might be helpful to also add a reference to 661447 (in both comments) Done.
+slan@, servolk@, phajdan.jr@ to CC / as FYI https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn#n... content/test/BUILD.gn:741: "//testing/buildbot/filters/cast-linux.content_browsertests.filter", On 2016/11/03 01:12:14, alexmos wrote: > I wonder how the Cast Linux bot manages to work, since it seems to use this > filter file as well as swarming, but before this, the filter file wasn't > included as a dependency. Not sure :-( Adding slan@ and servolk@ and phajdan.jr@ to CC in case they want to chime in. FWIW, the changes in the CL under review should only make things better here.
lukasza@chromium.org changed reviewers: + sky@chromium.org
sky@, can you take a look please? (BTW: I was a little surprised that both c/test/BUILD.gn files are owned by * - that might be fine, but I wanted to raise this to you [as the c/test/OWNER] in case it is also surprising to you)
sky@chromium.org changed reviewers: + brettw@chromium.org
This looks reasonable to me, but I'm not sure if there is some reason that we shouldn't always include the data deps. Brett likely knows better than I. So, sky->brettw
The CQ bit was checked by lukasza@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...
brettw@, can you take a look please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:1278: # as a dependency. Can you mention here what these files are used for? I wasn't aware of them before this. https://codereview.chromium.org/2469353002/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:739: data += [ It seems bad to duplicate this. Can you add a BUILD.gn file to the filters directory? You should be able to make a source set with data = [ ... ] in it and then reference it in the dependencies from the targets you need it for.
The CQ bit was checked by lukasza@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...
Thanks for review - can you take another look please? https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:1278: # as a dependency. On 2016/11/04 22:13:09, brettw (ping on IM after 24h) wrote: > Can you mention here what these files are used for? I wasn't aware of them > before this. I am adding a //testing/buildbot/filters/README.md in https://codereview.chromium.org/2472153002 - this is probably better than adding a one-off comment here, right? https://codereview.chromium.org/2469353002/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:739: data += [ On 2016/11/04 22:13:09, brettw (ping on IM after 24h) wrote: > It seems bad to duplicate this. Can you add a BUILD.gn file to the filters > directory? Done. > You should be able to make a source set with data = [ ... ] in it and then > reference it in the dependencies from the targets you need it for. Okay. Initially I wondered about using "group", but I see in the docs that it doesn't support "data" variable, so it is probably indeed safer + better to go with source sets.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by lukasza@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...
Thanks. sky@, could you please take a look at and stamp the changes in testing/buildbot/chromium.fyi.json?
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2469353002/#ps80001 (title: "Rebasing to minimize chance of merge conflicts (and of another https://crbug.com/661447).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The failure in linux_chromium_browser_side_navigation_rel is in layout tests and therefore 1) unlikely to be related to the CL under review and 2) failing because retry without patch step seems broken for linux_chromium_browser_side_navigation_rel. Therefore I am just going to land after removing linux_chromium_browser_side_navigation_rel from CQ_INCLUDE_TRYBOTS.
Message was sent while issue was closed.
Description was changed from ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/e8b43a499acd3df1ff12f08f2652d4acca2695fd Cr-Commit-Position: refs/heads/master@{#430603} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e8b43a499acd3df1ff12f08f2652d4acca2695fd Cr-Commit-Position: refs/heads/master@{#430603}
Message was sent while issue was closed.
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filter... File testing/buildbot/filters/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filter... testing/buildbot/filters/BUILD.gn:31: source_set("content_browsertests") { Can we rename these source sets? They prevent content_browsertests from being identified as a single target because they share an unqualified name -- i.e., to build content_browsertests, we now have to do ninja content/test:content_browsertests
Message was sent while issue was closed.
On 2016/11/09 18:20:44, jbudorick wrote: > https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filter... > File testing/buildbot/filters/BUILD.gn (right): > > https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filter... > testing/buildbot/filters/BUILD.gn:31: source_set("content_browsertests") { > Can we rename these source sets? They prevent content_browsertests from being > identified as a single target because they share an unqualified name -- i.e., to > build content_browsertests, we now have to do > > ninja content/test:content_browsertests Yes - sorry about that. This is being discussed in https://crbug.com/663785
Message was sent while issue was closed.
On 2016/11/09 18:32:11, Łukasz Anforowicz wrote: > On 2016/11/09 18:20:44, jbudorick wrote: > > > https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filter... > > File testing/buildbot/filters/BUILD.gn (right): > > > > > https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filter... > > testing/buildbot/filters/BUILD.gn:31: source_set("content_browsertests") { > > Can we rename these source sets? They prevent content_browsertests from being > > identified as a single target because they share an unqualified name -- i.e., > to > > build content_browsertests, we now have to do > > > > ninja content/test:content_browsertests > > Yes - sorry about that. This is being discussed in https://crbug.com/663785 Ah, ok. Thanks!
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2488963003/ by lukasza@chromium.org. The reason for reverting is: amineer@ points out that https://crbug.com/663785 also affects Canary and Dev builds - the safest / fastest thing to do seems to be to just revert.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
