Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401033003:
Fix gn installer/linux/BUILD.gn with target_arch=arm (Closed)
DescriptionFix gn installer/linux/BUILD.gn with target_arch=arm
Widevine is only available for x86 and x64 on linux. This
check was mistakenly removed in:
https://codereview.chromium.org/2395673002
The widevine shared libraries are not buildable for
other architectures currently (see
//third_party/widevine/cdm/BUILD.gn).
TEST=gn gen with target_cpu=arm and enable_linux_installer = true
BUG=653900
Committed: https://crrev.com/8c9a63e1b68dccd8dab4e4b093dcea3f31d60163
Cr-Commit-Position: refs/heads/master@{#424198}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 8
Patch Set 4 : . #Patch Set 5 : . #Messages
Total messages: 43 (24 generated)
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm BUG=653900 ==========
sbc@chromium.org changed reviewers: + glevin@chromium.org, thestig@chromium.org
On 2016/10/07 17:17:48, Sam Clegg wrote: > mailto:sbc@chromium.org changed reviewers: > + mailto:glevin@chromium.org, mailto:thestig@chromium.org LGTM, with the caveat that I don't even know what widevine is, and so know nothing about the possible ramifications of this change beyond (presumably) fixing the builders.
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is not limited to just x86 and x64 as far as I can tell. Of course, we don't officially support building the chrome debian package for arm, but that is not reason to add extra conditional to this file. BUG=653900 ==========
thestig@chromium.org changed reviewers: + ddorwin@chromium.org, xhwang@chromium.org
adding more reviewers
>Widevine is not limited to just x86 and x64 as far as I can tell. There is no CDM binary for Linux ARM (only Chrome OS ARM). Thus, the widevinecdm and widevinecdmadapter .so's will not exist. I'd have to look at whether those targets can "build" without producing anything, but if the installer expects those binaries to exist, they won't.
On 2016/10/07 18:00:55, ddorwin wrote: > >Widevine is not limited to just x86 and x64 as far as I can tell. > > There is no CDM binary for Linux ARM (only Chrome OS ARM). Thus, the widevinecdm > and widevinecdmadapter .so's will not exist. > > I'd have to look at whether those targets can "build" without producing > anything, but if the installer expects those binaries to exist, they won't. Those targets seem to build and produce nothing, so I think that unconditioanlly including them in deps is probably ok. I'm building the arm linux version of chromium now to see how far it gets building the debian package. It at least gets past the gn step which means that immediate issues is fixed by this change.
On 2016/10/07 18:10:28, Sam Clegg wrote: > On 2016/10/07 18:00:55, ddorwin wrote: > > >Widevine is not limited to just x86 and x64 as far as I can tell. > > > > There is no CDM binary for Linux ARM (only Chrome OS ARM). Thus, the > widevinecdm > > and widevinecdmadapter .so's will not exist. > > > > I'd have to look at whether those targets can "build" without producing > > anything, but if the installer expects those binaries to exist, they won't. > > Those targets seem to build and produce nothing, so I think that unconditioanlly > including them in deps is probably ok. I'm building the arm linux version of > chromium now to see how far it gets building the debian package. It at least > gets past the gn step which means that immediate issues is fixed by this change. Out of interest, is there some difference between Linux ARM and ChromeOS ARM that means the binaries won't "just work"?
On 2016/10/07 18:11:41, Sam Clegg wrote: > On 2016/10/07 18:10:28, Sam Clegg wrote: > > On 2016/10/07 18:00:55, ddorwin wrote: > > > >Widevine is not limited to just x86 and x64 as far as I can tell. > > > > > > There is no CDM binary for Linux ARM (only Chrome OS ARM). Thus, the > > widevinecdm > > > and widevinecdmadapter .so's will not exist. > > > > > > I'd have to look at whether those targets can "build" without producing > > > anything, but if the installer expects those binaries to exist, they won't. > > > > Those targets seem to build and produce nothing, so I think that > unconditioanlly > > including them in deps is probably ok. I'm building the arm linux version of > > chromium now to see how far it gets building the debian package. It at least > > gets past the gn step which means that immediate issues is fixed by this > change. Yes, it looks like that path results in a `group` that makes it a NOP. LGTM > > Out of interest, is there some difference between Linux ARM and ChromeOS ARM > that means the binaries won't "just work"? We also have separate binaries for x86 and x64. The binaries are different and the Chrome code is slightly different, but I don't know whether they could work.
lgtm
Please update the description. Widevine _is_ limited to those, but we don't need to limit the build dependency. (You can probably just say the latter.)
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is not limited to just x86 and x64 as far as I can tell. Of course, we don't officially support building the chrome debian package for arm, but that is not reason to add extra conditional to this file. BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine deps are setup to work on all architectures with null builds for unsupported architectures. This means we don't need the extra conditionals in this file. BUG=653900 ==========
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine deps are setup to work on all architectures with null builds for unsupported architectures. This means we don't need the extra conditionals in this file. BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine deps are setup to work on all architectures with null builds for unsupported architectures. This means we don't need the extra conditionals in this file. TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ==========
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine deps are setup to work on all architectures with null builds for unsupported architectures. This means we don't need the extra conditionals in this file. TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ==========
Simpler fix, which restores the previous behavior. PTAL.
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ==========
The CQ bit was checked by sbc@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...
On 2016/10/07 20:40:26, Sam Clegg wrote: > Simpler fix, which restores the previous behavior. PTAL. Still not knowing anything about this code, this LGTM even more :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... File chrome/installer/linux/BUILD.gn (right): https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:194: if (current_cpu == "x86" || current_cpu == "x64") { Maybe we should remove this since it does build and it'd be better to have less conditionals. WDYT? https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:241: if (is_chrome_branded && (current_cpu == "x86" || current_cpu == "x64")) { You might explain this in a comment. For example, the Widevine BUILD.gn file only produces binaries for these architectures. https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:243: "$root_out_dir/$widevine_cdm_path/libwidevinecdmadapter.so", I wonder if this should be the output of the build dependencies rather than explicitly specified here. There is some issue with moving/copying the files, though. +xhwang.
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ==========
https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... File chrome/installer/linux/BUILD.gn (right): https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:194: if (current_cpu == "x86" || current_cpu == "x64") { On 2016/10/07 22:37:52, ddorwin wrote: > Maybe we should remove this since it does build and it'd be better to have less > conditionals. WDYT? +1 https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:243: "$root_out_dir/$widevine_cdm_path/libwidevinecdmadapter.so", On 2016/10/07 22:37:52, ddorwin wrote: > I wonder if this should be the output of the build dependencies rather than > explicitly specified here. There is some issue with moving/copying the files, > though. +xhwang. I was following other examples in this file and I am not sure how we can get the output files by depending on the cdm targets.
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see //third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ==========
https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... File chrome/installer/linux/BUILD.gn (right): https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:194: if (current_cpu == "x86" || current_cpu == "x64") { On 2016/10/07 22:37:52, ddorwin wrote: > Maybe we should remove this since it does build and it'd be better to have less > conditionals. WDYT? I agree, but I'd like to keep this CL minimal to address the specific build failure. https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:241: if (is_chrome_branded && (current_cpu == "x86" || current_cpu == "x64")) { On 2016/10/07 22:37:52, ddorwin wrote: > You might explain this in a comment. For example, the Widevine BUILD.gn file > only produces binaries for these architectures. Done. https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... chrome/installer/linux/BUILD.gn:243: "$root_out_dir/$widevine_cdm_path/libwidevinecdmadapter.so", On 2016/10/07 22:37:52, ddorwin wrote: > I wonder if this should be the output of the build dependencies rather than > explicitly specified here. There is some issue with moving/copying the files, > though. +xhwang. Sounds like a good idea, but something for another CL perhaps?
On 2016/10/07 22:51:48, Sam Clegg wrote: > https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... > File chrome/installer/linux/BUILD.gn (right): > > https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... > chrome/installer/linux/BUILD.gn:194: if (current_cpu == "x86" || current_cpu == > "x64") { > On 2016/10/07 22:37:52, ddorwin wrote: > > Maybe we should remove this since it does build and it'd be better to have > less > > conditionals. WDYT? > > I agree, but I'd like to keep this CL minimal to address the specific build > failure. > > https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... > chrome/installer/linux/BUILD.gn:241: if (is_chrome_branded && (current_cpu == > "x86" || current_cpu == "x64")) { > On 2016/10/07 22:37:52, ddorwin wrote: > > You might explain this in a comment. For example, the Widevine BUILD.gn file > > only produces binaries for these architectures. > > Done. > > https://codereview.chromium.org/2401033003/diff/40001/chrome/installer/linux/... > chrome/installer/linux/BUILD.gn:243: > "$root_out_dir/$widevine_cdm_path/libwidevinecdmadapter.so", > On 2016/10/07 22:37:52, ddorwin wrote: > > I wonder if this should be the output of the build dependencies rather than > > explicitly specified here. There is some issue with moving/copying the files, > > though. +xhwang. > > Sounds like a good idea, but something for another CL perhaps? OK to land this as is?
It doesn't look like you uploaded the comment. LGTM with that. Please clean up the other items in a separate CL.
The CQ bit was checked by sbc@chromium.org to run a CQ dry run
On 2016/10/08 02:25:08, ddorwin wrote: > It doesn't look like you uploaded the comment. LGTM with that. Please clean up > the other items in a separate CL. Sorry, I though you were referring to the commit message. Comment added. CQ bit checked.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sbc@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sbc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, glevin@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2401033003/#ps80001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see //third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see //third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see //third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 ========== to ========== Fix gn installer/linux/BUILD.gn with target_arch=arm Widevine is only available for x86 and x64 on linux. This check was mistakenly removed in: https://codereview.chromium.org/2395673002 The widevine shared libraries are not buildable for other architectures currently (see //third_party/widevine/cdm/BUILD.gn). TEST=gn gen with target_cpu=arm and enable_linux_installer = true BUG=653900 Committed: https://crrev.com/8c9a63e1b68dccd8dab4e4b093dcea3f31d60163 Cr-Commit-Position: refs/heads/master@{#424198} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8c9a63e1b68dccd8dab4e4b093dcea3f31d60163 Cr-Commit-Position: refs/heads/master@{#424198} |
