Skip to content

Commit

Permalink
Revert "Only allow mappings for ICU initialization. (flutter#8656)" (f…
Browse files Browse the repository at this point in the history
…lutter#8682)

This reverts commit bd8c5b1.

Reverts flutter#8656

Reason:  flutter#8656 seems to break the framework windows tests and the engine roll (see https://cirrus-ci.com/task/4704667236827136 and flutter/flutter#31330). The failure has been consistent for 7 consecutive engine-to-framework auto-rolls.

TBR: @chinmaygarde
  • Loading branch information
liyuqian authored Apr 22, 2019
1 parent b4ed303 commit 1c9457c
Show file tree
Hide file tree
Showing 12 changed files with 125 additions and 61 deletions.
3 changes: 1 addition & 2 deletions benchmarking/benchmarking.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ namespace benchmarking {

int Main(int argc, char** argv) {
benchmark::Initialize(&argc, argv);
fml::icu::InitializeICU(fml::FileMapping::CreateReadOnly(
fml::OpenDirectoryOfExecutable(), "icudtl.dat"));
fml::icu::InitializeICU("icudtl.dat");
::benchmark::RunSpecifiedBenchmarks();
return 0;
}
Expand Down
6 changes: 6 additions & 0 deletions common/settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,12 @@ struct Settings {
bool verbose_logging = false;
std::string log_tag = "flutter";

// The icu_initialization_required setting does not have a corresponding
// switch because it is intended to be decided during build time, not runtime.
// Some companies apply source modification here because their build system
// brings its own ICU data files.
bool icu_initialization_required = true;
std::string icu_data_path;
MappingCallback icu_mapper;

// Assets settings
Expand Down
11 changes: 0 additions & 11 deletions fml/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include "flutter/fml/file.h"

#include "flutter/fml/logging.h"
#include "flutter/fml/paths.h"

namespace fml {

Expand Down Expand Up @@ -59,14 +58,4 @@ ScopedTemporaryDirectory::~ScopedTemporaryDirectory() {
}
}

fml::UniqueFD OpenDirectoryOfExecutable() {
auto result = paths::GetExecutableDirectoryPath();

if (!result.first) {
return {};
}

return OpenFile(result.second.c_str(), false, FilePermission::kRead);
}

} // namespace fml
2 changes: 0 additions & 2 deletions fml/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ fml::UniqueFD OpenDirectory(const fml::UniqueFD& base_directory,
bool create_if_necessary,
FilePermission permission);

fml::UniqueFD OpenDirectoryOfExecutable();

fml::UniqueFD Duplicate(fml::UniqueFD::element_type descriptor);

bool IsDirectory(const fml::UniqueFD& directory);
Expand Down
107 changes: 99 additions & 8 deletions fml/icu_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,115 @@

#include "flutter/fml/icu_util.h"

#include <memory>
#include <mutex>

#include "flutter/fml/build_config.h"
#include "flutter/fml/logging.h"
#include "flutter/fml/mapping.h"
#include "flutter/fml/native_library.h"
#include "flutter/fml/paths.h"
#include "third_party/icu/source/common/unicode/udata.h"

namespace fml {
namespace icu {

void InitializeICU(std::unique_ptr<const Mapping> mapping) {
if (mapping == nullptr || mapping->GetSize() == 0) {
return;
class ICUContext {
public:
ICUContext(const std::string& icu_data_path) : valid_(false) {
valid_ = SetupMapping(icu_data_path) && SetupICU();
}
static std::once_flag g_icu_init_flag;
std::call_once(g_icu_init_flag, [mapping = std::move(mapping)]() mutable {
static auto icu_mapping = std::move(mapping);

ICUContext(std::unique_ptr<Mapping> mapping) : mapping_(std::move(mapping)) {
valid_ = SetupICU();
}

~ICUContext() = default;

bool SetupMapping(const std::string& icu_data_path) {
// Check if the path exists and it readable directly.
auto fd =
fml::OpenFile(icu_data_path.c_str(), false, fml::FilePermission::kRead);

// Check the path relative to the current executable.
if (!fd.is_valid()) {
auto directory = fml::paths::GetExecutableDirectoryPath();

if (!directory.first) {
return false;
}

std::string path_relative_to_executable =
paths::JoinPaths({directory.second, icu_data_path});

fd = fml::OpenFile(path_relative_to_executable.c_str(), false,
fml::FilePermission::kRead);
}

if (!fd.is_valid()) {
return false;
}

std::initializer_list<FileMapping::Protection> protection = {
fml::FileMapping::Protection::kRead};

auto file_mapping =
std::make_unique<FileMapping>(fd, std::move(protection));

if (file_mapping->GetSize() != 0) {
mapping_ = std::move(file_mapping);
return true;
}

return false;
}

bool SetupICU() {
if (GetSize() == 0) {
return false;
}

UErrorCode err_code = U_ZERO_ERROR;
udata_setCommonData(icu_mapping->GetMapping(), &err_code);
FML_CHECK(err_code == U_ZERO_ERROR) << "Must be able to initialize ICU.";
udata_setCommonData(GetMapping(), &err_code);
return (err_code == U_ZERO_ERROR);
}

const uint8_t* GetMapping() const {
return mapping_ ? mapping_->GetMapping() : nullptr;
}

size_t GetSize() const { return mapping_ ? mapping_->GetSize() : 0; }

bool IsValid() const { return valid_; }

private:
bool valid_;
std::unique_ptr<Mapping> mapping_;

FML_DISALLOW_COPY_AND_ASSIGN(ICUContext);
};

void InitializeICUOnce(const std::string& icu_data_path) {
static ICUContext* context = new ICUContext(icu_data_path);
FML_CHECK(context->IsValid())
<< "Must be able to initialize the ICU context. Tried: " << icu_data_path;
}

std::once_flag g_icu_init_flag;
void InitializeICU(const std::string& icu_data_path) {
std::call_once(g_icu_init_flag,
[&icu_data_path]() { InitializeICUOnce(icu_data_path); });
}

void InitializeICUFromMappingOnce(std::unique_ptr<Mapping> mapping) {
static ICUContext* context = new ICUContext(std::move(mapping));
FML_CHECK(context->IsValid())
<< "Unable to initialize the ICU context from a mapping.";
}

void InitializeICUFromMapping(std::unique_ptr<Mapping> mapping) {
std::call_once(g_icu_init_flag, [mapping = std::move(mapping)]() mutable {
InitializeICUFromMappingOnce(std::move(mapping));
});
}

Expand Down
4 changes: 3 additions & 1 deletion fml/icu_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
namespace fml {
namespace icu {

void InitializeICU(std::unique_ptr<const Mapping> mapping);
void InitializeICU(const std::string& icu_data_path = "");

void InitializeICUFromMapping(std::unique_ptr<Mapping> mapping);

} // namespace icu
} // namespace fml
Expand Down
10 changes: 8 additions & 2 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,14 @@ static void PerformInitializationTasks(const Settings& settings) {
FML_DLOG(INFO) << "Skia deterministic rendering is enabled.";
}

if (settings.icu_mapper) {
fml::icu::InitializeICU(settings.icu_mapper());
if (settings.icu_initialization_required) {
if (settings.icu_data_path.size() != 0) {
fml::icu::InitializeICU(settings.icu_data_path);
} else if (settings.icu_mapper) {
fml::icu::InitializeICUFromMapping(settings.icu_mapper());
} else {
FML_DLOG(WARNING) << "Skipping ICU initialization in the shell.";
}
}
});
}
Expand Down
15 changes: 2 additions & 13 deletions shell/common/switches.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,20 +242,9 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) {
command_line.GetOptionValue(FlagForSwitch(Switch::CacheDirPath),
&settings.temp_directory_path);

{
// ICU from a data file.
std::string icu_data_path;
if (settings.icu_initialization_required) {
command_line.GetOptionValue(FlagForSwitch(Switch::ICUDataFilePath),
&icu_data_path);
if (icu_data_path.size() > 0) {
settings.icu_mapper = [icu_data_path]() {
return fml::FileMapping::CreateReadOnly(icu_data_path);
};
}
}

{
// ICU from a symbol in a dynamic library
&settings.icu_data_path);
if (command_line.HasOption(FlagForSwitch(Switch::ICUSymbolPrefix))) {
std::string icu_symbol_prefix, native_lib_path;
command_line.GetOptionValue(FlagForSwitch(Switch::ICUSymbolPrefix),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,10 @@
// defaults.

// Flutter ships the ICU data file in the the bundle of the engine. Look for it there.
if (!settings.icu_mapper) {
if (settings.icu_data_path.size() == 0) {
NSString* icuDataPath = [engineBundle pathForResource:@"icudtl" ofType:@"dat"];
if (icuDataPath.length > 0) {
auto icu_data_path = std::string{icuDataPath.UTF8String};
settings.icu_mapper = [icu_data_path]() {
return fml::FileMapping::CreateReadOnly(icu_data_path);
};
settings.icu_data_path = icuDataPath.UTF8String;
}
}

Expand Down
7 changes: 1 addition & 6 deletions shell/platform/embedder/embedder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,7 @@ FlutterEngineResult FlutterEngineRun(size_t version,

PopulateSnapshotMappingCallbacks(args, settings);

if (!settings.icu_mapper) {
settings.icu_mapper = [icu_data_path]() {
return fml::FileMapping::CreateReadOnly(icu_data_path);
};
}

settings.icu_data_path = icu_data_path;
settings.assets_path = args->assets_path;

if (!flutter::DartVM::IsRunningPrecompiledCode()) {
Expand Down
10 changes: 2 additions & 8 deletions shell/testing/tester_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,8 @@ int main(int argc, char* argv[]) {
return EXIT_FAILURE;
}

// Using command line arguments, the user can specify the ICU data as being
// present in either a file or a dynamic library. If no such specification has
// been, default to icudtl.dat.
if (!settings.icu_mapper) {
settings.icu_mapper = []() {
return fml::FileMapping::CreateReadOnly(fml::OpenDirectoryOfExecutable(),
"icudtl.dat");
};
if (settings.icu_data_path.size() == 0) {
settings.icu_data_path = "icudtl.dat";
}

// The tools that read logs get confused if there is a log tag specified.
Expand Down
4 changes: 1 addition & 3 deletions third_party/txt/tests/txt_run_all_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ int main(int argc, char** argv) {
}
FML_DCHECK(txt::GetFontDir().length() > 0);

fml::icu::InitializeICU(fml::FileMapping::CreateReadOnly(
fml::OpenDirectoryOfExecutable(), "icudtl.dat"));

fml::icu::InitializeICU("icudtl.dat");
SkGraphics::Init();
testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
Expand Down

0 comments on commit 1c9457c

Please sign in to comment.