Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1063453004:
Copy Widevine files for swarming tests (Closed)
DescriptionCopy Widevine files for swarming tests
BUG=474674
Committed: https://crrev.com/f584802a283f812bdeb9d3704c021a266486ab00
Cr-Commit-Position: refs/heads/master@{#324170}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Check branding #Patch Set 3 : remove extra quote #
Total comments: 9
Patch Set 4 : Android script #
Total comments: 4
Patch Set 5 : sort #
Total comments: 2
Messages
Total messages: 26 (5 generated)
jrummell@chromium.org changed reviewers: + thakis@chromium.org, xhwang@chromium.org
PTAL. Is this only used for official builds, or does Chromium use it as well, as Widevine only available on Chrome?
I'm not sure this is correct. From what I can tell, these libs only get built if
'(branding == "Chrome" or enable_widevine == 1) and enable_pepper_cdms == 1', {
(
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/widevi...
).
(I'm not sure if all these variables are accessible in isolates – see
https://code.google.com/p/chromium/codesearch#chromium/src/build/isolate.gypi...
for available variables; you can add more there if needed)
https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolate
File chrome/browser_tests.isolate (right):
https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolat...
chrome/browser_tests.isolate:185: '<(PRODUCT_DIR)/widevinecdmadapter.plugin',
add trailing /
On 2015/04/07 20:13:59, Nico wrote:
> I'm not sure this is correct. From what I can tell, these libs only get built
if
>
>
> '(branding == "Chrome" or enable_widevine == 1) and enable_pepper_cdms == 1',
{
…yup, that's why linux_chromium_compile_dbg_32 is red I think.
>
> (
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/widevi...
> ).
>
> (I'm not sure if all these variables are accessible in isolates – see
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/isolate.gypi...
> for available variables; you can add more there if needed)
>
> https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolate
> File chrome/browser_tests.isolate (right):
>
>
https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolat...
> chrome/browser_tests.isolate:185: '<(PRODUCT_DIR)/widevinecdmadapter.plugin',
> add trailing /
Updated. enable_widevine flag will not be needed. https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolate File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolat... chrome/browser_tests.isolate:185: '<(PRODUCT_DIR)/widevinecdmadapter.plugin', On 2015/04/07 20:13:58, Nico wrote: > add trailing / Done. Does clearkey above need it too?
lgtm I'm wondering if it's maybe better to have a "enable_widevine" gyp variable that's used in both the gyp file for widewine and in the isolate files, to make sure the conditions in the gyp and the isolate don't become out of sync…but it's probably ok as-is and since it's all in one repo it's easy to change to a variable if that's necessary in the future. https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolate File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/1/chrome/browser_tests.isolat... chrome/browser_tests.isolate:185: '<(PRODUCT_DIR)/widevinecdmadapter.plugin', On 2015/04/07 20:26:01, jrummell wrote: > On 2015/04/07 20:13:58, Nico wrote: > > add trailing / > > Done. Does clearkey above need it too? I don't think it's strictly needed, but it's nicer as it documents that the thing is really a folder https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:44: ['OS=="linux" and branding == "Chrome" and enable_pepper_cdms == 1', { no spaces around == https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:193: ['OS=="mac" and branding == "Chrome" and enable_pepper_cdms == 1', { likewise https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:214: ['OS=="win" and branding == "Chrome" and enable_pepper_cdms == 1', { likewise
maruel@chromium.org changed reviewers: + maruel@chromium.org
not lgtm Please update build/android/pylib/isolator.py. I forget the exact name, please grep for "isolate.py". https://codereview.chromium.org/1063453004/diff/40001/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1063453004/diff/40001/build/isolate.gypi#newc... build/isolate.gypi:103: '--config-variable', 'branding=<(branding)', Remove https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:44: ['OS=="linux" and branding == "Chrome" and enable_pepper_cdms == 1', { On 2015/04/07 20:36:33, Nico wrote: > no spaces around == Why branding=="Chrome" is needed?
https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:44: ['OS=="linux" and branding == "Chrome" and enable_pepper_cdms == 1', { On 2015/04/07 20:57:51, M-A Ruel wrote: > On 2015/04/07 20:36:33, Nico wrote: > > no spaces around == > > Why branding=="Chrome" is needed? Because libwidewinecdm.so only exists in that case, see upthread discussion and bug link.
Comments only. Not sure why I need to update Android files. Widevine is built in to Android, so no additional files need to be copied. https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:44: ['OS=="linux" and branding == "Chrome" and enable_pepper_cdms == 1', { On 2015/04/07 20:57:51, M-A Ruel wrote: > On 2015/04/07 20:36:33, Nico wrote: > > no spaces around == > > Why branding=="Chrome" is needed? Widevine is only available with Chrome, and not Chromium. What check should I use instead?
On 2015/04/07 21:06:02, jrummell wrote: > Not sure why I need to update Android files. Widevine is built in to Android, so > no additional files need to be copied. Good question. The .isolate files are used in two contexts, one when using through gyp for archival, one when running android tests to know what to map on the device. All the condition variables must be declared at both isolate.py call sites, parsing a .isolate with undefined variable will intentionally fail. Even if browser_tests is currently not used on Android, we always pass the exact same variables for consistency and simplify long term maintenance. With GN this will hopefully change but we're not there yet. https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:44: ['OS=="linux" and branding == "Chrome" and enable_pepper_cdms == 1', { On 2015/04/07 21:06:01, jrummell wrote: > On 2015/04/07 20:57:51, M-A Ruel wrote: > > On 2015/04/07 20:36:33, Nico wrote: > > > no spaces around == > > > > Why branding=="Chrome" is needed? > > Widevine is only available with Chrome, and not Chromium. What check should I > use instead? As Nico said, it would have been great if there was a single value but it's fine if this works so ignore my comment.
Updated with Android script. https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/40001/chrome/browser_tests.is... chrome/browser_tests.isolate:44: ['OS=="linux" and branding == "Chrome" and enable_pepper_cdms == 1', { On 2015/04/07 20:36:33, Nico wrote: > no spaces around == Done.
https://codereview.chromium.org/1063453004/diff/60001/build/android/pylib/uti... File build/android/pylib/utils/isolator.py (right): https://codereview.chromium.org/1063453004/diff/60001/build/android/pylib/uti... build/android/pylib/utils/isolator.py:48: 'branding': 'none', please sort use chromium instead or whatever default it is in build/common.gypi https://codereview.chromium.org/1063453004/diff/60001/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1063453004/diff/60001/build/isolate.gypi#newc... build/isolate.gypi:103: '--config-variable', 'branding=<(branding)', Please sort here too
Updated. https://codereview.chromium.org/1063453004/diff/60001/build/android/pylib/uti... File build/android/pylib/utils/isolator.py (right): https://codereview.chromium.org/1063453004/diff/60001/build/android/pylib/uti... build/android/pylib/utils/isolator.py:48: 'branding': 'none', On 2015/04/07 22:03:20, M-A Ruel wrote: > please sort > use chromium instead or whatever default it is in build/common.gypi Done. https://codereview.chromium.org/1063453004/diff/60001/build/isolate.gypi File build/isolate.gypi (right): https://codereview.chromium.org/1063453004/diff/60001/build/isolate.gypi#newc... build/isolate.gypi:103: '--config-variable', 'branding=<(branding)', On 2015/04/07 22:03:20, M-A Ruel wrote: > Please sort here too Done.
beautiful, lgtm.
Thanks for the reviews.
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1063453004/#ps80001 (title: "sort")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063453004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f584802a283f812bdeb9d3704c021a266486ab00 Cr-Commit-Position: refs/heads/master@{#324170}
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1063453004/diff/80001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/80001/chrome/browser_tests.is... chrome/browser_tests.isolate:197: '<(PRODUCT_DIR)/widevinecdmadapter.plugin/', This line breaks chrome mac compile: http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/bu...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1069273002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Breaks chrome mac compilation: http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/bu....
Message was sent while issue was closed.
On 2015/04/08 07:21:23, Michael Achenbach wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/1069273002/ by mailto:machenbach@chromium.org. > > The reason for reverting is: [Sheriff] Breaks chrome mac compilation: > http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/bu.... FAILED: cd ../../chrome; export BUILT_PRODUCTS_DIR=/Volumes/data/b/build/slave/google-chrome-rel-mac/build/src/out/Release; export CONFIGURATION=Release; export PRODUCT_NAME=browser_tests_run; export SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk; export SRCROOT=/Volumes/data/b/build/slave/google-chrome-rel-mac/build/src/out/Release/../../chrome; export SOURCE_ROOT="${SRCROOT}"; export TARGET_BUILD_DIR=/Volumes/data/b/build/slave/google-chrome-rel-mac/build/src/out/Release; export TEMP_DIR="${TMPDIR}";python ../tools/isolate_driver.py check --isolated "../out/Release/browser_tests.isolated" --isolate "browser_tests.isolate" --path-variable DEPTH .. --path-variable PRODUCT_DIR "../out/Release " --extra-variable "version_full=44.0.2360.0" --config-variable "CONFIGURATION_NAME=Release" --config-variable "OS=mac" --config-variable "asan=0" --config-variable "branding=Chrome" --config-variable "chromeos=0" --config-variable "component=static_library" --config-variable "disable_nacl=0" --config-variable "enable_pepper_cdms=1" --config-variable "enable_plugins=1" --config-variable "fastbuild=0" --config-variable "icu_use_data_file_flag=1" --config-variable "internal_gles2_conform_tests=0" --config-variable "libpeer_target_type=static_library" --config-variable "lsan=0" --config-variable "msan=0" --config-variable "target_arch=x64" --config-variable "tsan=0" --config-variable "use_custom_libcxx=0" --config-variable "use_instrumented_libraries=0" --config-variable "use_prebuilt_instrumented_libraries=0" --config-variable "use_openssl=1" --config-variable "use_ozone=0" --config-variable "use_x11=0" --config-variable "v8_use_external_startup_data=1" --extra-variable mac_product_name "Google Chrome" Failed to find an input file: /Volumes/data/b/build/slave/google-chrome-rel-mac/build/src/out/Release/widevinecdmadapter.plugin/ is not a directory but ends with "/" Huh, looks like this is called .plugin but isn't a directory? You can reland thsi without the trailing /, but medium term the file should be called something that doesn't end in ".plugin" if it isn't a bundle.
Message was sent while issue was closed.
https://codereview.chromium.org/1063453004/diff/80001/chrome/browser_tests.is... File chrome/browser_tests.isolate (right): https://codereview.chromium.org/1063453004/diff/80001/chrome/browser_tests.is... chrome/browser_tests.isolate:197: '<(PRODUCT_DIR)/widevinecdmadapter.plugin/', On 2015/04/08 07:20:50, Michael Achenbach wrote: > This line breaks chrome mac compile: > http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/bu... jrummell: Remove the trailing "/" and we should be fine... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
