From 3fef522496001b50a49b68bfe488d474206192f8 Mon Sep 17 00:00:00 2001 From: Filip Filmar Date: Wed, 22 Jul 2020 21:36:19 +0000 Subject: [PATCH] Revert "Reland "[vm] Replaces fuchsia.deprecatedtimezone"" This reverts commit e3a682480aad59de0de678856a2c968d8d6665d5. Reason for revert: Broke time reporting on new devices. Original change's description: > Reland "[vm] Replaces fuchsia.deprecatedtimezone" > > This is a reland of 16f09f20b326b0d7c285e7424650db45faca96e6 > > The apparent break of internal tests was not caused by this change. > > Original change's description: > > [vm] Replaces fuchsia.deprecatedtimezone > > > > (prior attempt was rolled back as it caused downstream tests to time > > out. See prior attempt at: See: > > https://dart-review.googlesource.com/c/sdk/+/149206) > > > > The FIDL library fuchsia.deprecatedtimezone is going away. There are > > different and better ways to obtain the same functionality. This change > > removes the dependency on fuchsia.deprecatedtimezone from the Dart SDK. > > > > Adds inspect metrics that allow whitebox testing of the runners. Here's > > a sample `fx iquery` excerpt from a running device, showing both a dart > > and a flutter runner exposing the same OS diagnostic metrics. > > > > ``` > > /hub/c/dart_jit_runner.cmx/70981/out/diagnostics: > > /hub/c/dart_jit_runner.cmx/70981/out/diagnostics#os: > > dst_status = 0 > > get_profile_status = 0 > > timezone_content_status = 0 > > tz_data_close_status = 0 > > tz_data_status = 0 > > /hub/c/flutter_jit_runner.cmx/29567/out/diagnostics: > > /hub/c/flutter_jit_runner.cmx/29567/out/diagnostics#os: > > dst_status = 0 > > get_profile_status = 0 > > timezone_content_status = 0 > > tz_data_close_status = 0 > > tz_data_status = 0 > > ``` > > > > Under nominal operation, all of the above values should be equal to 0. > > Nonzero values indicate an error. > > > > This functionality is guarded by Fuchsia integration tests at > > //src/tests/intl. > > > > Tested: > > (compile locally for Fuchsia and deploy) > > fx test //src/tests/intl > > > > See: > > - https://github.com/dart-lang/sdk/issues/42245 > > - https://github.com/dart-lang/sdk/issues/39650 > > > > Fixes #39650 > > > > Change-Id: I97f6e17e57000f6eec71246aee670bca65b7e1d1 > > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150662 > > Commit-Queue: Filip Filmar > > Reviewed-by: Martin Kustermann > > Change-Id: I5da6b0f481af0eb42c3b5e74c920588ac2ef5be9 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151862 > Reviewed-by: Martin Kustermann > Commit-Queue: Filip Filmar TBR=kustermann@google.com,kaushikiska@google.com,fmil@google.com Change-Id: I6e590cf22347f9153e5203b255f37872dbd91fa6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155505 Commit-Queue: Filip Filmar Reviewed-by: Filip Filmar --- runtime/vm/BUILD.gn | 8 ++- runtime/vm/os_fuchsia.cc | 123 ++++++++------------------------------- 2 files changed, 28 insertions(+), 103 deletions(-) diff --git a/runtime/vm/BUILD.gn b/runtime/vm/BUILD.gn index 3c042b173e6..0d1d129fcd2 100644 --- a/runtime/vm/BUILD.gn +++ b/runtime/vm/BUILD.gn @@ -74,7 +74,7 @@ library_for_all_configs("libdart_vm") { if (is_fuchsia) { if (using_fuchsia_gn_sdk) { extra_deps = [ - "$fuchsia_sdk_root/fidl/fuchsia.intl", + "$fuchsia_sdk_root/fidl/fuchsia.deprecatedtimezone", "$fuchsia_sdk_root/pkg/async", "$fuchsia_sdk_root/pkg/async-default", "$fuchsia_sdk_root/pkg/async-loop", @@ -87,7 +87,7 @@ library_for_all_configs("libdart_vm") { ] } else if (using_fuchsia_sdk) { extra_deps = [ - "$fuchsia_sdk_root/fidl:fuchsia.intl", + "$fuchsia_sdk_root/fidl:fuchsia.deprecatedtimezone", "$fuchsia_sdk_root/pkg:async-loop", "$fuchsia_sdk_root/pkg:async-loop-default", "$fuchsia_sdk_root/pkg:inspect", @@ -98,7 +98,9 @@ library_for_all_configs("libdart_vm") { ] } else { extra_deps = [ - "//sdk/fidl/fuchsia.intl", + # TODO(US-399): Remove time_service specific code when it is no longer + # necessary. + "//sdk/fidl/fuchsia.deprecatedtimezone", "//sdk/lib/sys/cpp", "//sdk/lib/sys/inspect/cpp", "//zircon/public/lib/fbl", diff --git a/runtime/vm/os_fuchsia.cc b/runtime/vm/os_fuchsia.cc index 74775388b19..c30ee49cfdb 100644 --- a/runtime/vm/os_fuchsia.cc +++ b/runtime/vm/os_fuchsia.cc @@ -11,7 +11,7 @@ #include #include -#include +#include #include #include #include @@ -24,31 +24,17 @@ #include #include -#include "third_party/icu/source/common/unicode/errorcode.h" -#include "third_party/icu/source/i18n/unicode/timezone.h" - #include "platform/assert.h" -#include "platform/syslog.h" #include "platform/utils.h" #include "vm/zone.h" namespace { -static constexpr int32_t kMsPerSec = 1000; - // The data directory containing ICU timezone data files. static constexpr char kICUTZDataDir[] = "/config/data/tzdata/icu/44/le"; -// This is the general OK status. -static constexpr int32_t kOk = 0; - -// This status means that the error code is not initialized yet ("set" was not -// yet called). Error codes are usually either 0 (kOk), or negative. -static constexpr int32_t kUninitialized = 1; - // The status codes for tzdata file open and read. enum class TZDataStatus { - // The operation completed without error. OK = 0, // The open call for the tzdata file did not succeed. COULD_NOT_OPEN = -1, @@ -58,8 +44,6 @@ enum class TZDataStatus { // Adds a facility for introspecting timezone data errors. Allows insight into // the internal state of the VM even if error reporting facilities fail. -// -// Under normal operation, all metric values below should be zero. class InspectMetrics { public: // Does not take ownership of inspector. @@ -67,14 +51,9 @@ class InspectMetrics { : inspector_(inspector), root_(inspector_->GetRoot()), metrics_(root_.CreateChild("os")), - dst_status_(metrics_.CreateInt("dst_status", kUninitialized)), - tz_data_status_(metrics_.CreateInt("tz_data_status", kUninitialized)), - tz_data_close_status_( - metrics_.CreateInt("tz_data_close_status", kUninitialized)), - get_profile_status_( - metrics_.CreateInt("get_profile_status", kUninitialized)), - profiles_timezone_content_status_( - metrics_.CreateInt("timezone_content_status", kOk)) {} + dst_status_(metrics_.CreateInt("dst_status", 0)), + tz_data_status_(metrics_.CreateInt("tz_data_status", 0)), + tz_data_close_status_(metrics_.CreateInt("tz_data_close_status", 0)) {} // Sets the last status code for DST offset calls. void SetDSTOffsetStatus(zx_status_t status) { @@ -88,17 +67,6 @@ class InspectMetrics { tz_data_close_status_.Set(status); } - // Sets the last status code for the call to PropertyProvider::GetProfile. - void SetProfileStatus(zx_status_t status) { - get_profile_status_.Set(static_cast(status)); - } - - // Sets the last status seen while examining timezones returned from - // PropertyProvider::GetProfile. - void SetTimeZoneContentStatus(zx_status_t status) { - profiles_timezone_content_status_.Set(static_cast(status)); - } - private: // The inspector that all metrics are being reported into. inspect::Inspector* inspector_; @@ -117,15 +85,6 @@ class InspectMetrics { // The return code for the close() call for tzdata files. inspect::IntProperty tz_data_close_status_; - - // The return code of the GetProfile call in GetTimeZoneName. If this is - // nonzero, then os_fuchsia.cc reported a default timezone as a fallback. - inspect::IntProperty get_profile_status_; - - // U_ILLEGAL_ARGUMENT_ERROR(=1) if timezones read from ProfileProvider were - // incorrect. Otherwise 0. If this metric reports U_ILLEGAL_ARGUMENT_ERROR, - // the os_fuchsia.cc module reported a default timezone as a fallback. - inspect::IntProperty profiles_timezone_content_status_; }; // Initialized on OS:Init(), deinitialized on OS::Cleanup. @@ -177,85 +136,50 @@ intptr_t OS::ProcessId() { return static_cast(getpid()); } -// This is the default timezone returned if it could not be obtained. For -// Fuchsia, the default device timezone is always UTC. -static const char kDefaultTimezone[] = "UTC"; - // TODO(FL-98): Change this to talk to fuchsia.dart to get timezone service to // directly get timezone. // // Putting this hack right now due to CP-120 as I need to remove // component:ConnectToEnvironmentServices and this is the only thing that is // blocking it and FL-98 will take time. -static fuchsia::intl::PropertyProviderSyncPtr property_provider; +static fuchsia::deprecatedtimezone::TimezoneSyncPtr tz; static zx_status_t GetLocalAndDstOffsetInSeconds(int64_t seconds_since_epoch, int32_t* local_offset, int32_t* dst_offset) { - const char* timezone_id = OS::GetTimeZoneName(seconds_since_epoch); - std::unique_ptr timezone( - icu::TimeZone::createTimeZone(timezone_id)); - UErrorCode error = U_ZERO_ERROR; - const auto ms_since_epoch = - static_cast(kMsPerSec * seconds_since_epoch); - // The units of time that local_offset and dst_offset are returned from this - // function is, usefully, not documented, but it seems that the units are - // milliseconds. Add these variables here for clarity. - int32_t local_offset_ms = 0; - int32_t dst_offset_ms = 0; - timezone->getOffset(ms_since_epoch, /*local_time=*/false, local_offset_ms, - dst_offset_ms, error); - metrics->SetDSTOffsetStatus(error); - if (error != U_ZERO_ERROR) { - icu::ErrorCode icu_error; - icu_error.set(error); - Syslog::PrintErr("could not get DST offset: %s\n", icu_error.errorName()); - return ZX_ERR_INTERNAL; + zx_status_t status = tz->GetTimezoneOffsetMinutes(seconds_since_epoch * 1000, + local_offset, dst_offset); + metrics->SetDSTOffsetStatus(status); + if (status != ZX_OK) { + return status; } - // We must return offset in seconds, so convert. - *local_offset = local_offset_ms / kMsPerSec; - *dst_offset = dst_offset_ms / kMsPerSec; + *local_offset *= 60; + *dst_offset *= 60; return ZX_OK; } const char* OS::GetTimeZoneName(int64_t seconds_since_epoch) { // TODO(abarth): Handle time zone changes. - static const std::unique_ptr tz_name = - std::make_unique([]() -> std::string { - fuchsia::intl::Profile profile; - const zx_status_t status = property_provider->GetProfile(&profile); - metrics->SetProfileStatus(status); - if (status != ZX_OK) { - return kDefaultTimezone; - } - const std::vector& timezones = - profile.time_zones(); - if (timezones.empty()) { - metrics->SetTimeZoneContentStatus(U_ILLEGAL_ARGUMENT_ERROR); - // Empty timezone array is not up to fuchsia::intl spec. The serving - // endpoint is broken and should be fixed. - Syslog::PrintErr("got empty timezone value\n"); - return kDefaultTimezone; - } - return timezones[0].id; - }()); + static const auto* tz_name = new std::string([] { + std::string result; + tz->GetTimezoneId(&result); + return result; + }()); return tz_name->c_str(); } int OS::GetTimeZoneOffsetInSeconds(int64_t seconds_since_epoch) { - int32_t local_offset = 0; - int32_t dst_offset = 0; - const zx_status_t status = GetLocalAndDstOffsetInSeconds( + int32_t local_offset, dst_offset; + zx_status_t status = GetLocalAndDstOffsetInSeconds( seconds_since_epoch, &local_offset, &dst_offset); return status == ZX_OK ? local_offset + dst_offset : 0; } int OS::GetLocalTimeZoneAdjustmentInSeconds() { + int32_t local_offset, dst_offset; zx_time_t now = 0; zx_clock_get(ZX_CLOCK_UTC, &now); - int32_t local_offset = 0; - int32_t dst_offset = 0; - const zx_status_t status = GetLocalAndDstOffsetInSeconds( + zx_status_t status = GetLocalAndDstOffsetInSeconds( now / ZX_SEC(1), &local_offset, &dst_offset); return status == ZX_OK ? local_offset : 0; } @@ -279,7 +203,7 @@ int64_t OS::GetCurrentMonotonicFrequency() { } int64_t OS::GetCurrentMonotonicMicros() { - const int64_t ticks = GetCurrentMonotonicTicks(); + int64_t ticks = GetCurrentMonotonicTicks(); ASSERT(GetCurrentMonotonicFrequency() == kNanosecondsPerSecond); return ticks / kNanosecondsPerMicrosecond; } @@ -433,8 +357,7 @@ void OS::Init() { metrics = std::make_unique(component_inspector->inspector()); InitializeTZData(); - auto services = sys::ServiceDirectory::CreateFromNamespace(); - services->Connect(property_provider.NewRequest()); + context->svc()->Connect(tz.NewRequest()); } void OS::Cleanup() {