Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: revert filesystem::path changes #55015

Merged
merged 1 commit into from
Sep 21, 2024
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
28 changes: 11 additions & 17 deletions src/compile_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "node_internals.h"
#include "node_version.h"
#include "path.h"
#include "util.h"
#include "zlib.h"

namespace node {
Expand Down Expand Up @@ -219,14 +220,11 @@ CompileCacheEntry* CompileCacheHandler::GetOrInsert(
compiler_cache_store_.emplace(key, std::make_unique<CompileCacheEntry>());
auto* result = emplaced.first->second.get();

std::u8string cache_filename_u8 =
(compile_cache_dir_ / Uint32ToHex(key)).u8string();
result->code_hash = code_hash;
result->code_size = code_utf8.length();
result->cache_key = key;
result->cache_filename =
std::string(cache_filename_u8.begin(), cache_filename_u8.end()) +
".cache";
compile_cache_dir_ + kPathSeparator + Uint32ToHex(key);
result->source_filename = filename_utf8.ToString();
result->cache = nullptr;
result->type = type;
Expand Down Expand Up @@ -364,43 +362,40 @@ CompileCacheEnableResult CompileCacheHandler::Enable(Environment* env,
const std::string& dir) {
std::string cache_tag = GetCacheVersionTag();
std::string absolute_cache_dir_base = PathResolve(env, {dir});
std::filesystem::path cache_dir_with_tag =
std::filesystem::path(absolute_cache_dir_base) / cache_tag;
std::u8string cache_dir_with_tag_u8 = cache_dir_with_tag.u8string();
std::string cache_dir_with_tag_str(cache_dir_with_tag_u8.begin(),
cache_dir_with_tag_u8.end());
std::string cache_dir_with_tag =
Copy link
Member

Choose a reason for hiding this comment

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

You can minimize the diff/potential conflicts if this is named as cache_dir_with_tag_str

absolute_cache_dir_base + kPathSeparator + cache_tag;
CompileCacheEnableResult result;
Debug("[compile cache] resolved path %s + %s -> %s\n",
dir,
cache_tag,
cache_dir_with_tag_str);
cache_dir_with_tag);

if (UNLIKELY(!env->permission()->is_granted(
env,
permission::PermissionScope::kFileSystemWrite,
cache_dir_with_tag_str))) {
cache_dir_with_tag))) {
result.message = "Skipping compile cache because write permission for " +
cache_dir_with_tag_str + " is not granted";
cache_dir_with_tag + " is not granted";
result.status = CompileCacheEnableStatus::FAILED;
return result;
}

if (UNLIKELY(!env->permission()->is_granted(
env,
permission::PermissionScope::kFileSystemRead,
cache_dir_with_tag_str))) {
cache_dir_with_tag))) {
result.message = "Skipping compile cache because read permission for " +
cache_dir_with_tag_str + " is not granted";
cache_dir_with_tag + " is not granted";
result.status = CompileCacheEnableStatus::FAILED;
return result;
}

fs::FSReqWrapSync req_wrap;
int err = fs::MKDirpSync(
nullptr, &(req_wrap.req), cache_dir_with_tag_str, 0777, nullptr);
nullptr, &(req_wrap.req), cache_dir_with_tag, 0777, nullptr);
if (is_debug_) {
Debug("[compile cache] creating cache directory %s...%s\n",
cache_dir_with_tag_str,
cache_dir_with_tag,
err < 0 ? uv_strerror(err) : "success");
}
if (err != 0 && err != UV_EEXIST) {
Expand All @@ -410,7 +405,6 @@ CompileCacheEnableResult CompileCacheHandler::Enable(Environment* env,
return result;
}

compile_cache_dir_str_ = absolute_cache_dir_base;
result.cache_directory = absolute_cache_dir_base;
compile_cache_dir_ = cache_dir_with_tag;
result.status = CompileCacheEnableStatus::ENABLED;
Expand Down
6 changes: 2 additions & 4 deletions src/compile_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <cinttypes>
#include <filesystem>
#include <memory>
#include <string>
#include <string_view>
Expand Down Expand Up @@ -69,7 +68,7 @@ class CompileCacheHandler {
void MaybeSave(CompileCacheEntry* entry,
v8::Local<v8::Module> mod,
bool rejected);
std::string_view cache_dir() { return compile_cache_dir_str_; }
std::string_view cache_dir() { return compile_cache_dir_; }

private:
void ReadCacheFile(CompileCacheEntry* entry);
Expand All @@ -92,8 +91,7 @@ class CompileCacheHandler {
v8::Isolate* isolate_ = nullptr;
bool is_debug_ = false;

std::string compile_cache_dir_str_;
std::filesystem::path compile_cache_dir_;
std::string compile_cache_dir_;
std::unordered_map<uint32_t, std::unique_ptr<CompileCacheEntry>>
compiler_cache_store_;
};
Expand Down
6 changes: 2 additions & 4 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include <atomic>
#include <cinttypes>
#include <cstdio>
#include <filesystem>
#include <iostream>
#include <limits>
#include <memory>
Expand Down Expand Up @@ -730,8 +729,7 @@ std::string Environment::GetCwd(const std::string& exec_path) {

// This can fail if the cwd is deleted. In that case, fall back to
// exec_path.
return exec_path.substr(
0, exec_path.find_last_of(std::filesystem::path::preferred_separator));
return exec_path.substr(0, exec_path.find_last_of(kPathSeparator));
}

void Environment::add_refs(int64_t diff) {
Expand Down Expand Up @@ -2122,7 +2120,7 @@ size_t Environment::NearHeapLimitCallback(void* data,
dir = Environment::GetCwd(env->exec_path_);
}
DiagnosticFilename name(env, "Heap", "heapsnapshot");
std::string filename = (std::filesystem::path(dir) / (*name)).string();
std::string filename = dir + kPathSeparator + (*name);

Debug(env, DebugCategory::DIAGNOSTICS, "Start generating %s...\n", *name);

Expand Down
5 changes: 2 additions & 3 deletions src/inspector_profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "v8-inspector.h"

#include <cinttypes>
#include <filesystem>
#include <limits>
#include <sstream>
#include "simdutf.h"
Expand Down Expand Up @@ -249,7 +248,7 @@ void V8ProfilerConnection::WriteProfile(simdjson::ondemand::object* result) {

std::string filename = GetFilename();
DCHECK(!filename.empty());
std::string path = (std::filesystem::path(directory) / filename).string();
std::string path = directory + kPathSeparator + filename;

WriteResult(env_, path.c_str(), profile);
}
Expand Down Expand Up @@ -305,7 +304,7 @@ void V8CoverageConnection::WriteProfile(simdjson::ondemand::object* result) {

std::string filename = GetFilename();
DCHECK(!filename.empty());
std::string path = (std::filesystem::path(directory) / filename).string();
std::string path = directory + kPathSeparator + filename;

// Only insert source map cache when there's source map data at all.
if (!source_map_cache_v->IsUndefined()) {
Expand Down
18 changes: 12 additions & 6 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ using v8::Value;
# define S_ISDIR(mode) (((mode) & S_IFMT) == S_IFDIR)
#endif

#ifdef __POSIX__
constexpr char kPathSeparator = '/';
#else
const char* const kPathSeparator = "\\/";
#endif

inline int64_t GetOffset(Local<Value> value) {
return IsSafeJsInt(value) ? value.As<Integer>()->Value() : -1;
}
Expand Down Expand Up @@ -1741,9 +1747,9 @@ int MKDirpSync(uv_loop_t* loop,
return err;
}
case UV_ENOENT: {
auto filesystem_path = std::filesystem::path(next_path);
if (filesystem_path.has_parent_path()) {
std::string dirname = filesystem_path.parent_path().string();
std::string dirname =
next_path.substr(0, next_path.find_last_of(kPathSeparator));
if (dirname != next_path) {
req_wrap->continuation_data()->PushPath(std::move(next_path));
req_wrap->continuation_data()->PushPath(std::move(dirname));
} else if (req_wrap->continuation_data()->paths().empty()) {
Expand Down Expand Up @@ -1821,9 +1827,9 @@ int MKDirpAsync(uv_loop_t* loop,
break;
}
case UV_ENOENT: {
auto filesystem_path = std::filesystem::path(path);
if (filesystem_path.has_parent_path()) {
std::string dirname = filesystem_path.parent_path().string();
std::string dirname =
path.substr(0, path.find_last_of(kPathSeparator));
if (dirname != path) {
req_wrap->continuation_data()->PushPath(path);
req_wrap->continuation_data()->PushPath(std::move(dirname));
} else if (req_wrap->continuation_data()->paths().empty()) {
Expand Down
10 changes: 4 additions & 6 deletions src/node_modules.cc
Original file line number Diff line number Diff line change
Expand Up @@ -322,14 +322,13 @@ void BindingData::GetNearestParentPackageJSON(
BufferValue path_value(realm->isolate(), args[0]);
// Check if the path has a trailing slash. If so, add it after
// ToNamespacedPath() as it will be deleted by ToNamespacedPath()
bool slashCheck = path_value.ToStringView().ends_with(
std::filesystem::path::preferred_separator);
bool slashCheck = path_value.ToStringView().ends_with(kPathSeparator);

ToNamespacedPath(realm->env(), &path_value);

std::string path_value_str = path_value.ToString();
if (slashCheck) {
path_value_str.push_back(std::filesystem::path::preferred_separator);
path_value_str.push_back(kPathSeparator);
}

auto package_json =
Expand All @@ -349,14 +348,13 @@ void BindingData::GetNearestParentPackageJSONType(
BufferValue path_value(realm->isolate(), args[0]);
// Check if the path has a trailing slash. If so, add it after
// ToNamespacedPath() as it will be deleted by ToNamespacedPath()
bool slashCheck = path_value.ToStringView().ends_with(
std::filesystem::path::preferred_separator);
bool slashCheck = path_value.ToStringView().ends_with(kPathSeparator);

ToNamespacedPath(realm->env(), &path_value);

std::string path_value_str = path_value.ToString();
if (slashCheck) {
path_value_str.push_back(std::filesystem::path::preferred_separator);
path_value_str.push_back(kPathSeparator);
}

auto package_json =
Expand Down
6 changes: 2 additions & 4 deletions src/node_report.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include <cstring>
#include <ctime>
#include <cwctype>
#include <filesystem>
#include <fstream>

constexpr int NODE_REPORT_VERSION = 3;
Expand Down Expand Up @@ -887,9 +886,8 @@ std::string TriggerNodeReport(Isolate* isolate,
report_directory = per_process::cli_options->report_directory;
}
// Regular file. Append filename to directory path if one was specified
if (!report_directory.empty()) {
std::string pathname =
(std::filesystem::path(report_directory) / filename).string();
if (report_directory.length() > 0) {
std::string pathname = report_directory + kPathSeparator + filename;
outfile.open(pathname, std::ios::out | std::ios::binary);
} else {
outfile.open(filename, std::ios::out | std::ios::binary);
Expand Down
4 changes: 2 additions & 2 deletions src/path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
namespace node {

#ifdef _WIN32
constexpr bool IsPathSeparator(char c) noexcept {
constexpr bool IsPathSeparator(const char c) noexcept {
return c == '\\' || c == '/';
}
#else // POSIX
constexpr bool IsPathSeparator(char c) noexcept {
constexpr bool IsPathSeparator(const char c) noexcept {
return c == '/';
}
#endif // _WIN32
Expand Down
3 changes: 1 addition & 2 deletions src/path.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@

namespace node {

class Environment;
constexpr bool IsPathSeparator(const char c) noexcept;

constexpr bool IsPathSeparator(char c) noexcept;
std::string NormalizeString(const std::string_view path,
bool allowAboveRoot,
const std::string_view separator);
Expand Down
18 changes: 13 additions & 5 deletions src/permission/fs_permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@
namespace {

std::string WildcardIfDir(const std::string& res) noexcept {
auto path = std::filesystem::path(res);
auto file_status = std::filesystem::status(path);
if (file_status.type() == std::filesystem::file_type::directory) {
path /= "*";
uv_fs_t req;
Copy link
Member Author

Choose a reason for hiding this comment

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

@joyeecheung Is it safe to keep this function, or do we need this revert?

Copy link
Member

@joyeecheung joyeecheung Sep 20, 2024

Choose a reason for hiding this comment

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

Is std::filesystem::path(res) correct? From what I can tell, res contains UTF8-encoded data, and it's about to be passed into uv_fs_stat which also expects UTF8-encoded data. So std::filesystem::path(res) here is also going to crash on Windows when res contains code points that are encoded differently in UTF16 unless it's casted between u8string both ways.

int rc = uv_fs_stat(nullptr, &req, res.c_str(), nullptr);
if (rc == 0) {
const uv_stat_t* const s = static_cast<const uv_stat_t*>(req.ptr);
if ((s->st_mode & S_IFMT) == S_IFDIR) {
// add wildcard when directory
if (res.back() == node::kPathSeparator) {
return res + "*";
}
return res + node::kPathSeparator + "*";
}
}
return path.string();
uv_fs_req_cleanup(&req);
return res;
}

void FreeRecursivelyNode(
Expand Down
3 changes: 1 addition & 2 deletions src/permission/fs_permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#include "v8.h"

#include <filesystem>
#include <unordered_map>
#include "permission/permission_base.h"
#include "util.h"
Expand Down Expand Up @@ -107,7 +106,7 @@ class FSPermission final : public PermissionBase {
// path = /home/subdirectory
// child = subdirectory/*
if (idx >= path.length() &&
child->prefix[i] == std::filesystem::path::preferred_separator) {
child->prefix[i] == node::kPathSeparator) {
continue;
}

Expand Down
3 changes: 3 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

#include <array>
#include <bit>
#include <filesystem>
#include <limits>
#include <memory>
#include <optional>
Expand All @@ -57,6 +58,8 @@

namespace node {

constexpr char kPathSeparator = std::filesystem::path::preferred_separator;

#ifdef _WIN32
/* MAX_PATH is in characters, not bytes. Make sure we have enough headroom. */
#define PATH_MAX_BYTES (MAX_PATH * 4)
Expand Down
Loading