Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Don't double-convert include path encodings in ImpellerC #37408

Merged
merged 3 commits into from
Nov 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1124,6 +1124,7 @@ 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.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
Expand Down
1 change: 1 addition & 0 deletions impeller/compiler/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ impeller_component("compiler_unittests") {
"compiler_test.cc",
"compiler_test.h",
"compiler_unittests.cc",
"switches_unittests.cc",
]

deps = [
Expand Down
10 changes: 8 additions & 2 deletions impeller/compiler/switches.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be unit tested in compiler_unittests.cc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// 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::UniqueFD>(fml::OpenDirectoryReadOnly(
*working_directory, Utf8FromPath(include_dir_absolute).c_str()));
*working_directory, include_dir_absolute.string().c_str()));
if (!dir || !dir->is_valid()) {
continue;
}
Expand Down
36 changes: 36 additions & 0 deletions impeller/compiler/switches_unittests.cc
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions impeller/compiler/utilities.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down