CalendarAstronomer in ICU (private) needs to be used outside ICU |
||||||||
Issue descriptionSpun off from https://codereview.chromium.org/2887913004 : third_party/icu/BUILD.gn has # Meta target that includes both icuuc and icui18n. Most targets want both. # You can depend on the individually if you need to. group("icu") { public_deps = [ ":icui18n", ":icuuc", ] } Both icu18n and icuuc have sources{} and public{}. In the former, all cc files and private header files are listed. In the latter, all public header files are listed. With that, I thought I could prevent private header files listed in sources{} but not in public{} from being included by files outside ICU. Apparently, that's not the case given what https://codereview.chromium.org/2887913004 does. Brett, could you advise what's the right way to achieve what I described above? Thank you
,
May 26 2017
Actually, if we run:
gn gen /out/Release --check
W get:
ERROR at //ash/system/night_light/night_light_controller.cc:15:11: Including a
private header.
#include "third_party/icu/source/i18n/astro.h"
^----------------------------------
This file is private to the target //third_party/icu:icui18n
So I think it's already doing what you intended?
I believe the file astro.h should be made public. There's no other utilities in the codebase to calculate sunset sunrise times.
,
May 26 2017
,
May 26 2017
Thanks for checking with gn. Then, this bug as filed is invalid. (I thought trybots all passed. Taking another look at the CL and trybot results, I realized that some bots failed in "analysis step" because of the prevention mechansim) > I believe the file astro.h should be made public. It can be done in icu/BUILD.gn, but it's private to ICU (used for Islamic calendar calculation) and subject to change without any notice.The class hasn't been changed for years, but who knows. Anyway, I filed http://bugs.icu-project.org/trac/ticket/13220 . Let's see what the upstream thinks of that.
,
May 26 2017
In the meantime, do you know what Android uses for the same purpose?
,
May 26 2017
Surprisingly, in Android, they're actually using icu.impl.CalendarAstronomer!! The same private API I'm using. https://cs.corp.google.com/android/frameworks/base/services/core/java/com/android/server/twilight/TwilightService.java?is_navigation=1&l=288
,
May 26 2017
Ok. That's good to know. Then, we can make a stronger case in the upstream ! :-)
,
May 30 2017
Friendly ping. Any feedback from upstream yet?
,
May 31 2017
http://bugs.icu-project.org/trac/ticket/13220 : they said that they'd make it a provisional public API in ICU 60.x. (I'm still rather reluctant... I was thinking of implementing one based on a published algorithm..). Anyway, upstream would do it. (an exact function signature TBD.) So, I'll just make a change in BUILD.gn to make the header file public in the meantime. Please, be prepare to change the call site once ICU 60 is out this fall.
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/deps/icu.git/+/f5b27d170db4815f472586a8f333fa9d1e39506e commit f5b27d170db4815f472586a8f333fa9d1e39506e Author: Jungshik Shin <jshin@chromium.org> Date: Wed May 31 22:34:58 2017 Make CalendarAstronomer public This is necessary for Chrome OS to calculate sunset time. Bug: chromium:726654 Test: With https://codereview.chromium.org/2887913004 patched in, 'gn Change-Id: Icf6a8cd833f8afcd1a2d0ce874cf1c8c92fcba44 gen <out_dir> --check' does not complain. TBR=afakhry@chromium.org Reviewed-on: https://chromium-review.googlesource.com/520323 Reviewed-by: Jungshik Shin <jshin@chromium.org> [modify] https://crrev.com/f5b27d170db4815f472586a8f333fa9d1e39506e/BUILD.gn
,
May 31 2017
Made it public. https://chromium.googlesource.com/chromium/deps/icu/+/f5b27d170db4815f472586a8f333fa9d1e39506e You can DEPS-roll ICU to f5b27d17 as a part of your CL.
,
May 31 2017
,
May 31 2017
Thank you very much. I'll do it in a separate CL.
,
May 31 2017
Reassigning. I can't DEPS-roll because there's a syntax error in the BUILD.gn file.
ERROR at //third_party/icu/BUILD.gn:453:5: Invalid token.
// crbug.com/726654 : Make astro.h public until the upstream adds an API.
^
Comments should start with # instead
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/deps/icu.git/+/ae18d60831126f902d778541495194380ff77fd8 commit ae18d60831126f902d778541495194380ff77fd8 Author: Jungshik Shin <jshin@chromium.org> Date: Thu Jun 01 06:52:21 2017 Fix a typo with astro.h Bug: chromium:726654 Test: gn gen works without an error Review-Url: https://codereview.chromium.org/2913423002 . [modify] https://crrev.com/ae18d60831126f902d778541495194380ff77fd8/BUILD.gn
,
Jun 1 2017
Oops. Sorry about that. Fixed. Pls, roll ICU to ae18d608311
,
Jun 1 2017
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83ac521049226516631c8f4aabd42fa0e9b4d2a8 commit 83ac521049226516631c8f4aabd42fa0e9b4d2a8 Author: afakhry <afakhry@chromium.org> Date: Thu Jun 01 17:48:54 2017 Roll ICU to c844075a..ae18d608 Two changes: f5b27d17 Make CalendarAstronomer public ae18d608 Fix a typo with astro.h TBR=jshin@chromium.org BUG= 726654 Review-Url: https://codereview.chromium.org/2914933002 Cr-Commit-Position: refs/heads/master@{#476343} [modify] https://crrev.com/83ac521049226516631c8f4aabd42fa0e9b4d2a8/DEPS
,
Jun 1 2017
,
Jun 1 2017
BTW, ICU code may or may not work well for high latitude. I haven't checked. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by afakhry@chromium.org
, May 26 2017