From d45d1bbcadd8408f672649719bacd2525edaa7e3 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Mon, 7 Nov 2022 20:30:46 -0800 Subject: [PATCH 1/3] Don't double-convert paths from the native format for includes --- impeller/compiler/switches.cc | 10 ++++++++-- impeller/compiler/utilities.h | 6 ++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc index 4ca2a27d5c5e2..1e73352b484c9 100644 --- a/impeller/compiler/switches.cc +++ b/impeller/compiler/switches.cc @@ -127,11 +127,17 @@ Switches::Switches(const fml::CommandLine& command_line) // fml::OpenDirectoryReadOnly for Windows doesn't handle relative paths // beginning with `../` well, so we build an absolute path. + + // Get the current working directory as a utf8 encoded string. + // Note that the `include_dir_path` is already utf8 encoded, and so we + // mustn't attempt to double-convert it to utf8 lest multi-byte characters + // will become mangled. + auto cwd = Utf8FromPath(std::filesystem::current_path()); auto include_dir_absolute = std::filesystem::absolute( - std::filesystem::current_path() / include_dir_path); + std::filesystem::path(cwd) / include_dir_path); auto dir = std::make_shared(fml::OpenDirectoryReadOnly( - *working_directory, Utf8FromPath(include_dir_absolute).c_str())); + *working_directory, include_dir_absolute.string().c_str())); if (!dir || !dir->is_valid()) { continue; } diff --git a/impeller/compiler/utilities.h b/impeller/compiler/utilities.h index 7b05d30aef893..61af436a87401 100644 --- a/impeller/compiler/utilities.h +++ b/impeller/compiler/utilities.h @@ -13,6 +13,12 @@ namespace impeller { namespace compiler { +/// @brief Converts a native format path to a utf8 string. +/// +/// This utility uses `path::u8string()` to convert native paths to +/// utf8. If the given path doesn't match the underlying native path +/// format, and the native path format isn't utf8 (i.e. Windows, which +/// has utf16 paths), the path will get mangled. std::string Utf8FromPath(const std::filesystem::path& path); std::string InferShaderNameFromPath(std::string_view path); From 523e0bada9d134d6f6082206fe91005e3f7372d1 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 8 Nov 2022 13:45:32 -0800 Subject: [PATCH 2/3] Add test --- ci/licenses_golden/licenses_flutter | 1 + impeller/compiler/BUILD.gn | 1 + impeller/compiler/switches_unittests.cc | 36 +++++++++++++++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 impeller/compiler/switches_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 5cb3aed373664..b304797cc311d 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1123,6 +1123,7 @@ FILE: ../../../flutter/impeller/compiler/source_options.h FILE: ../../../flutter/impeller/compiler/spirv_sksl.cc FILE: ../../../flutter/impeller/compiler/spirv_sksl.h FILE: ../../../flutter/impeller/compiler/switches.cc +FILE: ../../../flutter/impeller/compiler/switches_unittests.cc FILE: ../../../flutter/impeller/compiler/switches.h FILE: ../../../flutter/impeller/compiler/types.cc FILE: ../../../flutter/impeller/compiler/types.h diff --git a/impeller/compiler/BUILD.gn b/impeller/compiler/BUILD.gn index 8512408321050..3098941bf611d 100644 --- a/impeller/compiler/BUILD.gn +++ b/impeller/compiler/BUILD.gn @@ -120,6 +120,7 @@ impeller_component("compiler_unittests") { "compiler_test.cc", "compiler_test.h", "compiler_unittests.cc", + "switches_unittests.cc", ] deps = [ diff --git a/impeller/compiler/switches_unittests.cc b/impeller/compiler/switches_unittests.cc new file mode 100644 index 0000000000000..54ca1e293d4da --- /dev/null +++ b/impeller/compiler/switches_unittests.cc @@ -0,0 +1,36 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/fml/command_line.h" +#include "flutter/fml/file.h" +#include "flutter/testing/testing.h" +#include "impeller/compiler/switches.h" +#include "impeller/compiler/utilities.h" + +namespace impeller { +namespace compiler { +namespace testing { + +TEST(SwitchesTest, DoesntMangleUnicodeIncludes) { + const char* directory_name = "test_shader_include_�"; + fml::CreateDirectory(flutter::testing::OpenFixturesDirectory(), + {directory_name}, fml::FilePermission::kRead); + + auto include_path = + std::string(flutter::testing::GetFixturesPath()) + "/" + directory_name; + auto include_option = "--include=" + include_path; + + const auto cl = fml::CommandLineFromInitializerList( + {"impellerc", "--opengl-desktop", "--input=input.vert", + "--sl=output.vert", "--spirv=output.spirv", include_option.c_str()}); + Switches switches(cl); + ASSERT_TRUE(switches.AreValid(std::cout)); + ASSERT_EQ(switches.include_directories.size(), 1u); + ASSERT_NE(switches.include_directories[0].dir, nullptr); + ASSERT_EQ(switches.include_directories[0].name, include_path); +} + +} // namespace testing +} // namespace compiler +} // namespace impeller From 28c498210ce0fbf2ba37691197ebc14fe85cdd8e Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 8 Nov 2022 14:02:24 -0800 Subject: [PATCH 3/3] Licenses --- ci/licenses_golden/licenses_flutter | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index b304797cc311d..0f6405ae83ce6 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1123,8 +1123,8 @@ FILE: ../../../flutter/impeller/compiler/source_options.h FILE: ../../../flutter/impeller/compiler/spirv_sksl.cc FILE: ../../../flutter/impeller/compiler/spirv_sksl.h FILE: ../../../flutter/impeller/compiler/switches.cc -FILE: ../../../flutter/impeller/compiler/switches_unittests.cc FILE: ../../../flutter/impeller/compiler/switches.h +FILE: ../../../flutter/impeller/compiler/switches_unittests.cc FILE: ../../../flutter/impeller/compiler/types.cc FILE: ../../../flutter/impeller/compiler/types.h FILE: ../../../flutter/impeller/compiler/utilities.cc