Skip to content

Commit

Permalink
[common] Remove filesystem shim (RobotLocomotion#18159)
Browse files Browse the repository at this point in the history
For files whose primary purpose is file manipulation, we use the
conventional 'namespace fs' alias for brevity. For files where
developers might not have seen that abbreviation yet, we stick with
the std::filesystem spelled out in full.
  • Loading branch information
jwnimmer-tri authored Oct 24, 2022
1 parent 3c42adb commit 419368d
Show file tree
Hide file tree
Showing 61 changed files with 234 additions and 322 deletions.
27 changes: 1 addition & 26 deletions common/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -337,23 +337,13 @@ drake_cc_library(
visibility = ["//tools/install/libdrake:__pkg__"],
)

# The drake::filesystem support is intended ONLY for use within Drake's *.cc
# files -- it is not a public dependency of Drake, so we mark it internal.
drake_cc_library(
name = "filesystem",
hdrs = ["filesystem.h"],
internal = True,
visibility = ["//:__subpackages__"],
)

drake_cc_library(
name = "find_runfiles",
srcs = ["find_runfiles.cc"],
hdrs = ["find_runfiles.h"],
interface_deps = [],
deps = [
":essential",
":filesystem",
"@bazel_tools//tools/cpp/runfiles",
],
)
Expand Down Expand Up @@ -381,7 +371,6 @@ drake_cc_library(
],
deps = [
":drake_marker_shared_library",
":filesystem",
":find_runfiles",
],
)
Expand All @@ -396,7 +385,6 @@ drake_cc_library(
visibility = ["//tools/install/libdrake:__pkg__"],
interface_deps = [],
deps = [
":filesystem",
":find_resource",
],
)
Expand Down Expand Up @@ -497,9 +485,7 @@ drake_cc_library(
interface_deps = [
":essential",
],
deps = [
":filesystem",
],
deps = [],
)

# This is a Drake-internal utility for use only as a direct dependency
Expand Down Expand Up @@ -920,22 +906,13 @@ drake_cc_googletest(
],
)

drake_cc_googletest(
name = "filesystem_test",
deps = [
":filesystem",
"//common/test_utilities:expect_throws_message",
],
)

drake_cc_googletest(
name = "find_resource_test",
data = [
"test/find_resource_test_data.txt",
],
deps = [
":drake_path",
":filesystem",
":find_resource",
"//common/test_utilities:expect_no_throw",
],
Expand Down Expand Up @@ -969,7 +946,6 @@ drake_cc_googletest(
"test/find_resource_test_data.txt",
],
deps = [
":filesystem",
":find_runfiles",
":temp_directory",
"//common/test_utilities:expect_throws_message",
Expand Down Expand Up @@ -1073,7 +1049,6 @@ drake_cc_googletest(
# changes from one accidentally polluting the others.
shard_count = 3,
deps = [
":filesystem",
":temp_directory",
],
)
Expand Down
5 changes: 3 additions & 2 deletions common/drake_path.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "drake/common/drake_path.h"

#include "drake/common/filesystem.h"
#include <filesystem>

#include "drake/common/find_resource.h"

// N.B. This code is unit tested in test/find_resource_test.cc.
Expand All @@ -15,7 +16,7 @@ std::optional<std::string> MaybeGetDrakePath() {
if (find_result.get_error_message()) {
return std::nullopt;
}
filesystem::path sentinel_path(find_result.get_absolute_path_or_throw());
std::filesystem::path sentinel_path(find_result.get_absolute_path_or_throw());

// Take the dirname of sentinel_path, so that we have a folder where looking
// up "drake/foo/bar.urdf" makes sense.
Expand Down
17 changes: 0 additions & 17 deletions common/filesystem.h

This file was deleted.

5 changes: 3 additions & 2 deletions common/find_loaded_library.cc
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#include "drake/common/find_loaded_library.h"

#include <filesystem>

#include "drake/common/drake_throw.h"
#include "drake/common/filesystem.h"

#ifdef __APPLE__
#include <dlfcn.h>
Expand Down Expand Up @@ -102,7 +103,7 @@ std::optional<string> LoadedLibraryPath(const std::string& library_name) {
if (pos_slash && !strcmp(pos_slash + 1, library_name.c_str())) {
// Check if path is relative. If so, make it absolute.
if (map->l_name[0] != '/') {
std::string argv0 = filesystem::read_symlink({
std::string argv0 = std::filesystem::read_symlink({
"/proc/self/exe"}).string();
return string(dirname(&argv0[0])) + "/" +
string(map->l_name, pos_slash - map->l_name);
Expand Down
18 changes: 10 additions & 8 deletions common/find_resource.cc
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
#include "drake/common/find_resource.h"

#include <cstdlib>
#include <filesystem>
#include <utility>
#include <vector>

#include <fmt/format.h>

#include "drake/common/drake_marker.h"
#include "drake/common/drake_throw.h"
#include "drake/common/filesystem.h"
#include "drake/common/find_loaded_library.h"
#include "drake/common/find_runfiles.h"
#include "drake/common/never_destroyed.h"
Expand All @@ -19,6 +19,8 @@ using std::string;

namespace drake {

namespace fs = std::filesystem;

using Result = FindResourceResult;

std::optional<string>
Expand Down Expand Up @@ -126,11 +128,11 @@ Result CheckAndMakeResult(
DRAKE_DEMAND(!root_description.empty());
DRAKE_DEMAND(!root.empty());
DRAKE_DEMAND(!resource_path.empty());
DRAKE_DEMAND(filesystem::is_directory({root}));
DRAKE_DEMAND(fs::is_directory({root}));
DRAKE_DEMAND(IsRelativePath(resource_path));

// Check for the sentinel.
if (!filesystem::is_regular_file({root + "/" + kSentinelRelpath})) {
if (!fs::is_regular_file({root + "/" + kSentinelRelpath})) {
return Result::make_error(resource_path, fmt::format(
"Could not find Drake resource_path '{}' because {} specified a "
"resource root of '{}' but that root did not contain the expected "
Expand All @@ -140,7 +142,7 @@ Result CheckAndMakeResult(

// Check for the resource_path.
const string abspath = root + '/' + resource_path;
if (!filesystem::is_regular_file({abspath})) {
if (!fs::is_regular_file({abspath})) {
return Result::make_error(resource_path, fmt::format(
"Could not find Drake resource_path '{}' because {} specified a "
"resource root of '{}' but that root did not contain the expected "
Expand All @@ -163,19 +165,19 @@ std::optional<string> MaybeGetEnvironmentResourceRoot() {
return std::nullopt;
}
const std::string root{env_value};
if (!filesystem::is_directory({root})) {
if (!fs::is_directory({root})) {
static const logging::Warn log_once(
"FindResource ignoring {}='{}' because it does not exist.",
env_name, env_value);
return std::nullopt;
}
if (!filesystem::is_directory({root + "/drake"})) {
if (!fs::is_directory({root + "/drake"})) {
static const logging::Warn log_once(
"FindResource ignoring {}='{}' because it does not contain a 'drake' "
"subdirectory.", env_name, env_value);
return std::nullopt;
}
if (!filesystem::is_regular_file({root + "/" + kSentinelRelpath})) {
if (!fs::is_regular_file({root + "/" + kSentinelRelpath})) {
static const logging::Warn log_once(
"FindResource ignoring {}='{}' because it does not contain the "
"expected sentinel file '{}'.", env_name, env_value, kSentinelRelpath);
Expand All @@ -193,7 +195,7 @@ std::optional<string> MaybeGetInstallResourceRoot() {
std::optional<string> libdrake_dir = LoadedLibraryPath("libdrake_marker.so");
if (libdrake_dir) {
const string root = *libdrake_dir + "/../share";
if (filesystem::is_directory({root})) {
if (fs::is_directory({root})) {
return root;
} else {
log()->debug("FindResource ignoring CMake install candidate '{}' "
Expand Down
19 changes: 11 additions & 8 deletions common/find_runfiles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@
#include <mach-o/dyld.h>
#endif

#include <filesystem>

#include "fmt/format.h"
#include "tools/cpp/runfiles/runfiles.h"

#include "drake/common/drake_assert.h"
#include "drake/common/filesystem.h"
#include "drake/common/never_destroyed.h"
#include "drake/common/text_logging.h"

Expand All @@ -21,6 +22,8 @@ using bazel::tools::cpp::runfiles::Runfiles;
namespace drake {
namespace {

namespace fs = std::filesystem;

// Replace `nullptr` with `"nullptr",` or else just return `arg` unchanged.
const char* nullable_to_string(const char* arg) {
return arg ? arg : "nullptr";
Expand Down Expand Up @@ -64,14 +67,14 @@ RunfilesSingleton Create() {
argv0 = argv0.c_str(); // Remove trailing nil bytes.
drake::log()->debug("_NSGetExecutablePath = {}", argv0);
#else
const std::string& argv0 = filesystem::read_symlink({
const std::string& argv0 = fs::read_symlink({
"/proc/self/exe"}).string();
drake::log()->debug("readlink(/proc/self/exe) = {}", argv0);
#endif
result.runfiles.reset(Runfiles::Create(argv0, &bazel_error));
}
drake::log()->debug("FindRunfile mechanism = {}", mechanism);
drake::log()->debug("cwd = {}", filesystem::current_path());
drake::log()->debug("cwd = {}", fs::current_path());

// If there were runfiles, identify the RUNFILES_DIR.
if (result.runfiles) {
Expand All @@ -82,8 +85,8 @@ RunfilesSingleton Create() {
// `bazel-bin/target`.
// TODO(eric.cousineau): Show this in Drake itself. This behavior was
// encountered in Anzu issue 5653, in a Python binary.
filesystem::path path = key_value.second;
path = filesystem::absolute(path);
fs::path path = key_value.second;
path = fs::absolute(path);
path = path.lexically_normal();
result.runfiles_dir = path.string();
break;
Expand All @@ -93,7 +96,7 @@ RunfilesSingleton Create() {
if (result.runfiles_dir.empty()) {
bazel_error = "RUNFILES_DIR was not provided by the Runfiles object";
result.runfiles.reset();
} else if (!filesystem::is_directory({result.runfiles_dir})) {
} else if (!fs::is_directory({result.runfiles_dir})) {
bazel_error = fmt::format(
"RUNFILES_DIR '{}' does not exist", result.runfiles_dir);
result.runfiles.reset();
Expand Down Expand Up @@ -163,8 +166,8 @@ RlocationOrError FindRunfile(const std::string& resource_path) {
// Locate the file on the manifest and in the directory.
const std::string by_man = singleton.runfiles->Rlocation(resource_path);
const std::string by_dir = singleton.runfiles_dir + "/" + resource_path;
const bool by_man_ok = filesystem::is_regular_file({by_man});
const bool by_dir_ok = filesystem::is_regular_file({by_dir});
const bool by_man_ok = fs::is_regular_file({by_man});
const bool by_dir_ok = fs::is_regular_file({by_dir});
drake::log()->debug(
"FindRunfile found by-manifest '{}' ({}) and by-directory '{}' ({})",
by_man, by_man_ok ? "good" : "bad", by_dir, by_dir_ok ? "good" : "bad");
Expand Down
1 change: 0 additions & 1 deletion common/proto/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ drake_cc_library(
interface_deps = [],
deps = [
"//common:essential",
"//common:filesystem",
],
)

Expand Down
6 changes: 3 additions & 3 deletions common/proto/rpc_pipe_temp_directory.cc
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
#include "drake/common/proto/rpc_pipe_temp_directory.h"

#include <cstdlib>
#include <filesystem>

#include "drake/common/drake_throw.h"
#include "drake/common/filesystem.h"

namespace drake {
namespace common {
Expand All @@ -12,8 +12,8 @@ std::string GetRpcPipeTempDirectory() {
const char* path_str = nullptr;
(path_str = std::getenv("TEST_TMPDIR")) || (path_str = "/tmp");

const filesystem::path path(path_str);
DRAKE_THROW_UNLESS(filesystem::is_directory(path));
const std::filesystem::path path(path_str);
DRAKE_THROW_UNLESS(std::filesystem::is_directory(path));
return path.string();
}

Expand Down
10 changes: 6 additions & 4 deletions common/temp_directory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@
#include <unistd.h>

#include <cstdlib>
#include <filesystem>

#include "drake/common/drake_throw.h"
#include "drake/common/filesystem.h"

namespace drake {

namespace fs = std::filesystem;

std::string temp_directory() {
filesystem::path path;
fs::path path;

const char* tmpdir = nullptr;
(tmpdir = std::getenv("TEST_TMPDIR")) || (tmpdir = std::getenv("TMPDIR")) ||
(tmpdir = "/tmp");

filesystem::path path_template(tmpdir);
fs::path path_template(tmpdir);
path_template.append("robotlocomotion_drake_XXXXXX");

std::string path_template_str = path_template.string();
Expand All @@ -25,7 +27,7 @@ std::string temp_directory() {

path = dtemp;

DRAKE_THROW_UNLESS(filesystem::is_directory(path));
DRAKE_THROW_UNLESS(fs::is_directory(path));
std::string path_string = path.string();
DRAKE_DEMAND(path_string.back() != '/');

Expand Down
31 changes: 0 additions & 31 deletions common/test/filesystem_test.cc

This file was deleted.

Loading

0 comments on commit 419368d

Please sign in to comment.