Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929873004:
Roll libwebm 75a6d2d:9a235e0 (Closed)
|
Created:
4 years, 7 months ago by Tom Finegan Modified:
4 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRoll libwebm 75a6d2d:9a235e0
Update BUILD.gn and libwebm.gyp for new libwebm source layout.
Upstream changes:
<too many to list>
BUG=601636
Committed: https://crrev.com/795f1a69a3f9f532ff11c7de0d23d34f6ffbd27e
Cr-Commit-Position: refs/heads/master@{#390587}
Patch Set 1 #Patch Set 2 : tweak include path in BUILD.gn config #Patch Set 3 : Add public_deps to BUILD.gn #Patch Set 4 : fix cflags in gyp/wrap -Wno-deprecated-declarations in !is_win #Patch Set 5 : wrap -Wno-deprecated-declarations properly/remove public_deps in BUILD.gn #Patch Set 6 : BUILD.gn: s/public_configs/all_dependent_configs/ #Patch Set 7 : maybe this is how we export an include path? #Patch Set 8 : We need configs AND public_configs set in BUILD.gn #Patch Set 9 : Append instead of assigning configs #Patch Set 10 : maybe? #Patch Set 11 : add libwebm to public_deps of media/BUILD.gn #
Total comments: 1
Patch Set 12 : Export libwebm dep from media.gyp #Patch Set 13 : add libwebm_config to media/BUILD.gn all_dependent_configs and remove libwebm from public_deps #
Total comments: 1
Patch Set 14 : add libwebm_config to media/BUILD.gn public_configs, remove libwebm from all_dependent_configs #Patch Set 15 : add libwebm to media/BUILD.gn public_deps; remove the config from public_configs #Patch Set 16 : move libwebm_config public_configs entry to media component of media/BUILD.gn #
Messages
Total messages: 54 (25 generated)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/1
Description was changed from ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> ========== to ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> ==========
tomfinegan@chromium.org changed reviewers: + mcasas@chromium.org
LGTM if the bots are happy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/20001
Description was changed from ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> ========== to ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> BUG=601636 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/140001
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by tomfinegan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Just one comment for discussion. https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn#newcode467 media/BUILD.gn:467: "//third_party/libwebm", Remove this? Actually, if you just need to expose the config, can you use public_configs instead? See example at https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/B...
On 2016/04/28 22:59:33, xhwang wrote: > Just one comment for discussion. > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn > File media/BUILD.gn (right): > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn#newcode467 > media/BUILD.gn:467: "//third_party/libwebm", > Remove this? > > Actually, if you just need to expose the config, can you use public_configs > instead? See example at > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/B... Trying the above, but are you OK with the media.gyp change?
On 2016/04/28 23:23:02, Tom Finegan wrote: > On 2016/04/28 22:59:33, xhwang wrote: > > Just one comment for discussion. > > > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn > > File media/BUILD.gn (right): > > > > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn#newcode467 > > media/BUILD.gn:467: "//third_party/libwebm", > > Remove this? > > > > Actually, if you just need to expose the config, can you use public_configs > > instead? See example at > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/B... > > Trying the above, but are you OK with the media.gyp change? well, assuming it works... :)
On 2016/04/28 23:23:23, Tom Finegan wrote: > On 2016/04/28 23:23:02, Tom Finegan wrote: > > On 2016/04/28 22:59:33, xhwang wrote: > > > Just one comment for discussion. > > > > > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn > > > File media/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn#newcode467 > > > media/BUILD.gn:467: "//third_party/libwebm", > > > Remove this? > > > > > > Actually, if you just need to expose the config, can you use public_configs > > > instead? See example at > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/B... > > > > Trying the above, but are you OK with the media.gyp change? > > well, assuming it works... :) So nice to see you again Tom!!! gpy change looks good. BTW gyp is going away really soon...
On 2016/04/28 23:26:35, xhwang wrote: > On 2016/04/28 23:23:23, Tom Finegan wrote: > > On 2016/04/28 23:23:02, Tom Finegan wrote: > > > On 2016/04/28 22:59:33, xhwang wrote: > > > > Just one comment for discussion. > > > > > > > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn > > > > File media/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1929873004/diff/190001/media/BUILD.gn#newcode467 > > > > media/BUILD.gn:467: "//third_party/libwebm", > > > > Remove this? > > > > > > > > Actually, if you just need to expose the config, can you use > public_configs > > > > instead? See example at > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/B... > > > > > > Trying the above, but are you OK with the media.gyp change? > > > > well, assuming it works... :) > > So nice to see you again Tom!!! > Likewise, it's been too long. Time to find an excuse to come work in Kirkland for a bit. :) > gpy change looks good. BTW gyp is going away really soon... Ah, good to know. Kind of a pain to have to keep two project files updated.
https://codereview.chromium.org/1929873004/diff/220001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1929873004/diff/220001/media/BUILD.gn#newcode234 media/BUILD.gn:234: "//third_party/libwebm:libwebm_config", Sorry, we have so many bad examples in this file for you to follow... We are trying to avoid all_dependent_configs as much as possible. This is strongly opposed by gn folks. So we should actually fix the :media_dependent_config entry we have. In your case, will public_configs work for you? If public_deps works, it should work.
On 2016/04/28 23:28:42, xhwang wrote: > https://codereview.chromium.org/1929873004/diff/220001/media/BUILD.gn > File media/BUILD.gn (right): > > https://codereview.chromium.org/1929873004/diff/220001/media/BUILD.gn#newcode234 > media/BUILD.gn:234: "//third_party/libwebm:libwebm_config", > Sorry, we have so many bad examples in this file for you to follow... > > We are trying to avoid all_dependent_configs as much as possible. This is > strongly opposed by gn folks. So we should actually fix the > :media_dependent_config entry we have. > > In your case, will public_configs work for you? If public_deps works, it should > work. public_configs won't work in media_dependent_config, but does when added at l.242 where public_deps is first declared https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_... It does seem to work when it's added within the media component itself. Is that ok? Bot trying it now to make sure.
On 2016/04/28 23:58:30, Tom Finegan wrote: > On 2016/04/28 23:28:42, xhwang wrote: > > https://codereview.chromium.org/1929873004/diff/220001/media/BUILD.gn > > File media/BUILD.gn (right): > > > > > https://codereview.chromium.org/1929873004/diff/220001/media/BUILD.gn#newcode234 > > media/BUILD.gn:234: "//third_party/libwebm:libwebm_config", > > Sorry, we have so many bad examples in this file for you to follow... > > > > We are trying to avoid all_dependent_configs as much as possible. This is > > strongly opposed by gn folks. So we should actually fix the > > :media_dependent_config entry we have. > > > > In your case, will public_configs work for you? If public_deps works, it > should > > work. > > public_configs won't work in media_dependent_config, but does when added at > l.242 where public_deps is first declared > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_... > > It does seem to work when it's added within the media component itself. Is that > ok? Bot trying it now to make sure. Yes, this is what I meant. LGTM!
The CQ bit was checked by tomfinegan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1929873004/#ps280001 (title: "move libwebm_config public_configs entry to media component of media/BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tomfinegan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929873004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929873004/280001
Message was sent while issue was closed.
Description was changed from ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> BUG=601636 ========== to ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> BUG=601636 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:280001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/795f1a69a3f9f532ff11c7de0d23d34f6ffbd27e Cr-Commit-Position: refs/heads/master@{#390587}
Message was sent while issue was closed.
Description was changed from ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> BUG=601636 ========== to ========== Roll libwebm 75a6d2d:9a235e0 Update BUILD.gn and libwebm.gyp for new libwebm source layout. Upstream changes: <too many to list> BUG=601636 Committed: https://crrev.com/795f1a69a3f9f532ff11c7de0d23d34f6ffbd27e Cr-Commit-Position: refs/heads/master@{#390587} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
