Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1710493002:
Speculative fix for Google3 iOS build (Closed)
DescriptionSpeculative fix for Google3 iOS build
Now that SkImageGenerator is hooked up to use SkCodec by default,
we need to compile SkCodec in order to compile Skia.
The good news is that we can now turn on/off individual codecs.
This CL enables SkCodec on iOS, but does not turn on any of the
codecs that require third_party libraries.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1710493002
Committed: https://skia.googlesource.com/skia/+/2775cf548ca62d3b1076a7e9cc2a40853f1bb885
Patch Set 1 #Patch Set 2 : Exclude srcs with dependencies #
Total comments: 2
Patch Set 3 : #Patch Set 4 : Rebase #Patch Set 5 : Adding changes from the diff #Patch Set 6 : Rebase again #Messages
Total messages: 24 (8 generated)
Description was changed from ========== Speculative fix for Google3 iOS build Now that SkImageGenerator is hooked up to use SkCodec by default, we need to compile SkCodec in order to compile Skia. The good news is that we can now turn on/off individual codecs. This CL enables SkCodec on iOS, but does not turn on any of the codecs that require third_party libraries. BUG=skia: ========== to ========== Speculative fix for Google3 iOS build Now that SkImageGenerator is hooked up to use SkCodec by default, we need to compile SkCodec in order to compile Skia. The good news is that we can now turn on/off individual codecs. This CL enables SkCodec on iOS, but does not turn on any of the codecs that require third_party libraries. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + dogben@google.com, tomhudson@google.com
On 2016/02/17 at 18:53:22, msarett wrote: > Hmm, that's odd. I ran a full presubmit with your last CL and it passed. Working on testing this fix. Note that https://codereview.chromium.org/1703923002/ may conflict.
Here is the CL that did the breaking... https://codereview.chromium.org/1699183004/ The one you reviewed was probably fine.
Fingers crossed on this one...
benjaminwagner@google.com changed reviewers: + benjaminwagner@google.com
https://codereview.chromium.org/1710493002/diff/20001/public.bzl File public.bzl (right): https://codereview.chromium.org/1710493002/diff/20001/public.bzl#newcode216 public.bzl:216: "src/codec/*Gif*", Can't exclude these here, because they're already included by BASE_SRCS_ALL, and these excludes don't apply to those includes.
Patch Set 3 fixes the issues with 2. https://codereview.chromium.org/1710493002/diff/20001/public.bzl File public.bzl (right): https://codereview.chromium.org/1710493002/diff/20001/public.bzl#newcode216 public.bzl:216: "src/codec/*Gif*", On 2016/02/17 20:00:58, Ben Wagner wrote: > Can't exclude these here, because they're already included by BASE_SRCS_ALL, and > these excludes don't apply to those includes. Oh gotcha - trying a new approach.
On 2016/02/17 at 20:09:22, msarett wrote: > Patch Set 3 fixes the issues with 2. > > https://codereview.chromium.org/1710493002/diff/20001/public.bzl > File public.bzl (right): > > https://codereview.chromium.org/1710493002/diff/20001/public.bzl#newcode216 > public.bzl:216: "src/codec/*Gif*", > On 2016/02/17 20:00:58, Ben Wagner wrote: > > Can't exclude these here, because they're already included by BASE_SRCS_ALL, and > > these excludes don't apply to those includes. > > Oh gotcha - trying a new approach. The following seems to work -- running all tests now. diff --git a/public.bzl b/public.bzl index 987d7c5..4ad89d5 100644 --- a/public.bzl +++ b/public.bzl @@ -217,6 +217,8 @@ BASE_SRCS_IOS = struct( include = [ "src/gpu/gl/GrGLDefaultInterface_native.cpp", "src/gpu/gl/iOS/GrGLCreateNativeInterface_iOS.cpp", + "src/codec/*", "src/opts/**/*.cpp", "src/opts/**/*.h", "src/ports/**/*.cpp", @@ -224,6 +226,13 @@ BASE_SRCS_IOS = struct( "src/utils/mac/*.cpp", ], exclude = [ + "src/codec/*Android*", + "src/codec/*Gif*.cpp", + "src/codec/*Ico*.cpp", + "src/codec/*Jpeg*.cpp", + "src/codec/*Webp*.cpp", + "src/codec/*Png*", + "src/codec/*Raw*.cpp", "src/opts/*mips*", "src/opts/*NEON*", "src/opts/*neon*",
Adding changes from the diff... Did you intentionally drop src/android/*? I'm not sure it will matter either way.
On 2016/02/17 at 20:32:09, msarett wrote: > Adding changes from the diff... > > Did you intentionally drop src/android/*? I'm not sure it will matter either way. I removed it when trying to get things to compile. I can try again with it added. (But it seems odd that the iOS build would use files in src/android.)
On 2016/02/17 at 21:01:35, Ben Wagner wrote: > On 2016/02/17 at 20:32:09, msarett wrote: > > Adding changes from the diff... > > > > Did you intentionally drop src/android/*? I'm not sure it will matter either way. > > I removed it when trying to get things to compile. I can try again with it added. (But it seems odd that the iOS build would use files in src/android.) Also, please rebase.
On 2016/02/17 21:01:35, Ben Wagner wrote: > On 2016/02/17 at 20:32:09, msarett wrote: > > Adding changes from the diff... > > > > Did you intentionally drop src/android/*? I'm not sure it will matter either > way. > > I removed it when trying to get things to compile. I can try again with it > added. (But it seems odd that the iOS build would use files in src/android.) I don't think it will make a difference (though I've been wrong before). I'd like to include it if that's ok with you. There is nothing Android specific about the code (except for the fact that Android is currently the only one who uses it).
lgtm LGTM with or without src/android.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1710493002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1710493002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-Arm7...) Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benjaminwagner@google.com Link to the patchset: https://codereview.chromium.org/1710493002/#ps100001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1710493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1710493002/100001
Message was sent while issue was closed.
Description was changed from ========== Speculative fix for Google3 iOS build Now that SkImageGenerator is hooked up to use SkCodec by default, we need to compile SkCodec in order to compile Skia. The good news is that we can now turn on/off individual codecs. This CL enables SkCodec on iOS, but does not turn on any of the codecs that require third_party libraries. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Speculative fix for Google3 iOS build Now that SkImageGenerator is hooked up to use SkCodec by default, we need to compile SkCodec in order to compile Skia. The good news is that we can now turn on/off individual codecs. This CL enables SkCodec on iOS, but does not turn on any of the codecs that require third_party libraries. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2775cf548ca62d3b1076a7e9cc2a40853f1bb885 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/2775cf548ca62d3b1076a7e9cc2a40853f1bb885 |
