Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1057153002:
content_browsertests.isolate: ppapi_tests requires enable_plugins=1. (Closed)
Descriptioncontent_browsertests.isolate: ppapi_tests requires enable_plugins=1.
R=cjhopman@chromium.org,maruel@chromium.org
BUG=472823
Committed: https://crrev.com/7623ad7130f4beb954b00192f8009de208ca84c0
Cr-Commit-Position: refs/heads/master@{#323826}
Patch Set 1 #
Total comments: 4
Patch Set 2 : sets disable_nacl for chromecast #Patch Set 3 : back to enable_plugins, updating isolate args #
Messages
Total messages: 22 (3 generated)
On 2015/04/03 00:42:24, gunsch wrote: Example failure found on cast_shell_linux: http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...
gunsch@chromium.org changed reviewers: + scottmg@chromium.org - thakis@chromium.org
+scottmg, looks like @thakis is OOO
https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi#newcode83 build/isolate.gypi:83: '--config-variable', 'enable_plugins=<(enable_plugins)', http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... I don't see enable_plugins here. Is nacl enabled at all? Because I'd prefer to key on disable_nacl==0.
https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi#newcode83 build/isolate.gypi:83: '--config-variable', 'enable_plugins=<(enable_plugins)', On 2015/04/03 00:45:26, M-A Ruel wrote: > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... > > I don't see enable_plugins here. > Is nacl enabled at all? Because I'd prefer to key on disable_nacl==0. enable_plugins is set to 0 when chromecast==1 (see embedded==1 in build/common.gypi). I chose enable_plugins because that's how it's linked into content_tests, so that's how it seems like it should be guarded in the .isolate file: https://code.google.com/p/chromium/codesearch#chromium/src/content/content_te... Should that condition also be disable_nacl?
You have to update build/android/setup.py or something (I always forget the name), grep for isolate.py in there https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi#newcode83 build/isolate.gypi:83: '--config-variable', 'enable_plugins=<(enable_plugins)', On 2015/04/03 00:48:38, gunsch wrote: > On 2015/04/03 00:45:26, M-A Ruel wrote: > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... > > > > I don't see enable_plugins here. > > Is nacl enabled at all? Because I'd prefer to key on disable_nacl==0. > > enable_plugins is set to 0 when chromecast==1 (see embedded==1 in > build/common.gypi). > > > I chose enable_plugins because that's how it's linked into content_tests, so > that's how it seems like it should be guarded in the .isolate file: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/content_te... > > Should that condition also be disable_nacl? My question is: is disable_nacl==0 set when chromecast==1. Because there's way more people desiring proper support for disable_nacl==0. I agree that enable_plugins is way more than just nacl but from the perspective of this specific file, it doesn't matter.
https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1057153002/diff/1/build/isolate.gypi#newcode83 build/isolate.gypi:83: '--config-variable', 'enable_plugins=<(enable_plugins)', On 2015/04/03 00:56:08, M-A Ruel wrote: > On 2015/04/03 00:48:38, gunsch wrote: > > On 2015/04/03 00:45:26, M-A Ruel wrote: > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... > > > > > > I don't see enable_plugins here. > > > Is nacl enabled at all? Because I'd prefer to key on disable_nacl==0. > > > > enable_plugins is set to 0 when chromecast==1 (see embedded==1 in > > build/common.gypi). > > > > > > I chose enable_plugins because that's how it's linked into content_tests, so > > that's how it seems like it should be guarded in the .isolate file: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/content_te... > > > > Should that condition also be disable_nacl? > > My question is: > is disable_nacl==0 set when chromecast==1. Because there's way more people > desiring proper support for disable_nacl==0. I agree that enable_plugins is way > more than just nacl but from the perspective of this specific file, it doesn't > matter. To make sure we're on the same page: do most builds have both "disable_nacl=0 and enable_plugins=1"? I'm not sure which value of disable_nacl you're asking ppapi_tests to be included in the .isolate for. We generally set disable_nacl=1 when chromecast=1, though I noticed we only do so in our internal build scripts (which tack on a lot more GYP_DEFINES). I've added disable_nacl=1 explicitly to build/common.gypi when chromecast=1, and then guarded it here in content_browsertests.isolate.
I prefer patchset 2, but you forgot: On 2015/04/03 00:56:08, M-A Ruel wrote: > You have to update build/android/setup.py or something (I always forget the > name), grep for isolate.py in there
On 2015/04/03 01:44:56, M-A Ruel wrote: > I prefer patchset 2, but you forgot: > > On 2015/04/03 00:56:08, M-A Ruel wrote: > > You have to update build/android/setup.py or something (I always forget the > > name), grep for isolate.py in there Sorry, I don't follow---I'm no longer adding a new variable to the isolate runner, what needs to be updated?
On 2015/04/03 01:48:03, gunsch wrote: > On 2015/04/03 01:44:56, M-A Ruel wrote: > > I prefer patchset 2, but you forgot: > > > > On 2015/04/03 00:56:08, M-A Ruel wrote: > > > You have to update build/android/setup.py or something (I always forget the > > > name), grep for isolate.py in there > > Sorry, I don't follow---I'm no longer adding a new variable to the isolate > runner, what needs to be updated? Oh you're right, it's just late and I just need to log off now. :) lgtm
On 2015/04/03 01:50:49, M-A Ruel wrote: > Oh you're right, it's just late and I just need to log off now. :) > > lgtm not lgtm, one sec I was confused by ppapi vs pnacl. Sorry about that, you were right about enable_plugin.
gunsch@chromium.org changed reviewers: + cjhopman@chromium.org - scottmg@chromium.org
On 2015/04/03 01:51:43, M-A Ruel wrote: > On 2015/04/03 01:50:49, M-A Ruel wrote: > > Oh you're right, it's just late and I just need to log off now. :) > > > > lgtm > > not lgtm, one sec > > I was confused by ppapi vs pnacl. Sorry about that, you were right about > enable_plugin. alright, back to enable_plugins, and updated the Android script.
lgtm
@maruel: patchset 3 works with android isolate change: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
thanks, lgtm. Sorry for the SNAFU, it's an holiday today in Canada and yesterday evening I was, uh, "distracted". :)
On 2015/04/03 21:27:28, M-A Ruel wrote: > thanks, lgtm. Sorry for the SNAFU, it's an holiday today in Canada and yesterday > evening I was, uh, "distracted". :) thanks for the review, enjoy your holiday!
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1057153002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7623ad7130f4beb954b00192f8009de208ca84c0 Cr-Commit-Position: refs/heads/master@{#323826} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
