Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2150603004:
Build Expat on all platforms (Closed)
DescriptionBuild Expat on all platforms
* bump version to @android-6.0.1_r55 (2.1.1)
* add as a dependency to the 'xml' target (for upcoming http://crrev.com/2142893006)
* tweak build for Mac, Win:
- define HAVE_EXPAT_CONFIG_H (same as in Android.mk)
- use -Wno-missing-field-initializers (same as in Android.mk)
- suppress MSVS warning 4244 (same as ..cmake/Utilities/cmexpat/expatConfig.h.in)
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2150603004
R=mtklein@google.com
Committed: https://skia.googlesource.com/skia/+/771a190a0021d95500d73d4f187e0ef2aa3a0fdc
Patch Set 1 #Patch Set 2 : bump version #Patch Set 3 : tweak build flags #Patch Set 4 : help locate expat_config.h #Patch Set 5 : fix clags #Patch Set 6 : disable msvs warning #Patch Set 7 : speculative Mac warnings fix #Patch Set 8 : tighten warning suppressions #
Total comments: 4
Patch Set 9 : trim obsolete comment #Messages
Total messages: 28 (19 generated)
Description was changed from ========== Expat TLC WIP ========== to ========== Expat TLC WIP GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2150603004 ==========
Description was changed from ========== Expat TLC WIP GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2150603004 ========== to ========== Build Expat on all platforms * bump version to @android-6.0.1_r55 (2.1.1) * add as a dependency to the 'xml' target (for upcoming http://crrev.com/2142893006) * tweak build for Mac, Win: - define HAVE_EXPAT_CONFIG_H (same as in Android.mk) - use -Wno-missing-field-initializers (same as in Android.mk) - suppress MSVS warning 4244 (same as ..cmake/Utilities/cmexpat/expatConfig.h.in) GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2150603004 R=mtklein@google.com ==========
fmalita@chromium.org changed reviewers: + mtklein@google.com
What am I missing here? I can't get Mac to pick up -DHAVE_EXPAT_CONFIG_H and -Wno-missing-field-initializers: https://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86... (but I can see them being set for local verbose Linux builds?!)
The CQ bit was checked by fmalita@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...
fmalita@chromium.org changed reviewers: + bungeman@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by fmalita@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/07/14 18:05:21, f(malita) wrote:
> What am I missing here?
Apparently an Xcode settings entry:
'xcode_settings': { 'WARNING_CFLAGS': [ '-Wno-missing-field-initializers', ], },
Seems to build fine now, PTAL.
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 fmalita@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...
https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp File gyp/xml.gyp (right): https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp#newcode14 gyp/xml.gyp:14: 'expat.gyp:expat', hmmm... skia_lib will already have expat built into it in non-framework android builds and linux builds (to parse fonts.xml). I'm not sure what kind of headaches that will cause, especially when building skia_lib as a shared lib.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp File gyp/xml.gyp (right): https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp#newcode14 gyp/xml.gyp:14: 'expat.gyp:expat', On 2016/07/14 19:30:04, bungeman-skia wrote: > hmmm... skia_lib will already have expat built into it in non-framework android > builds and linux builds (to parse fonts.xml). I'm not sure what kind of > headaches that will cause, especially when building skia_lib as a shared lib. Is that the rule in ports.gyp? Is it any different from having multiple static lib targets depend on a common component (expat static lib)? FWIW I tried a local Linux -Dskia_shared_lib=1 and didn't notice any issues.
https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp File gyp/xml.gyp (right): https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp#newcode14 gyp/xml.gyp:14: 'expat.gyp:expat', On 2016/07/14 19:54:35, f(malita) wrote: > On 2016/07/14 19:30:04, bungeman-skia wrote: > > hmmm... skia_lib will already have expat built into it in non-framework > android > > builds and linux builds (to parse fonts.xml). I'm not sure what kind of > > headaches that will cause, especially when building skia_lib as a shared lib. > > Is that the rule in ports.gyp? Is it any different from having multiple static > lib targets depend on a common component (expat static lib)? > > FWIW I tried a local Linux -Dskia_shared_lib=1 and didn't notice any issues. Or is it about the system libexpat, which gets linked in for fontconfig? Should we do something like this: https://cs.chromium.org/chromium/src/third_party/expat/expat.gyp?rcl=0&l=8
https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp File gyp/xml.gyp (right): https://codereview.chromium.org/2150603004/diff/140001/gyp/xml.gyp#newcode14 gyp/xml.gyp:14: 'expat.gyp:expat', On 2016/07/14 19:54:35, f(malita) wrote: > On 2016/07/14 19:30:04, bungeman-skia wrote: > > hmmm... skia_lib will already have expat built into it in non-framework > android > > builds and linux builds (to parse fonts.xml). I'm not sure what kind of > > headaches that will cause, especially when building skia_lib as a shared lib. > > Is that the rule in ports.gyp? Is it any different from having multiple static > lib targets depend on a common component (expat static lib)? > > FWIW I tried a local Linux -Dskia_shared_lib=1 and didn't notice any issues. Thinking about this really hard, we shouldn't run into any issues so long as we don't pass expat types across the boundary between libs (which we have no plans to do anyway). In the past we were always running into issues with SkString where if there are two implementations and an instance from one lib was used in the other lib bad things would happen (though we asserted on these). Apart from the duplication of compilation, lgtm.
The CQ bit was checked by fmalita@chromium.org
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 ========== Build Expat on all platforms * bump version to @android-6.0.1_r55 (2.1.1) * add as a dependency to the 'xml' target (for upcoming http://crrev.com/2142893006) * tweak build for Mac, Win: - define HAVE_EXPAT_CONFIG_H (same as in Android.mk) - use -Wno-missing-field-initializers (same as in Android.mk) - suppress MSVS warning 4244 (same as ..cmake/Utilities/cmexpat/expatConfig.h.in) GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2150603004 R=mtklein@google.com ========== to ========== Build Expat on all platforms * bump version to @android-6.0.1_r55 (2.1.1) * add as a dependency to the 'xml' target (for upcoming http://crrev.com/2142893006) * tweak build for Mac, Win: - define HAVE_EXPAT_CONFIG_H (same as in Android.mk) - use -Wno-missing-field-initializers (same as in Android.mk) - suppress MSVS warning 4244 (same as ..cmake/Utilities/cmexpat/expatConfig.h.in) GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2150603004 R=mtklein@google.com Committed: https://skia.googlesource.com/skia/+/771a190a0021d95500d73d4f187e0ef2aa3a0fdc ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/771a190a0021d95500d73d4f187e0ef2aa3a0fdc
Message was sent while issue was closed.
djsollen@google.com changed reviewers: + djsollen@google.com
Message was sent while issue was closed.
this CL breaks the automerger into Android. :( https://chromegw.corp.google.com/i/client.skia.internal/builders/Housekeeper-... |
