diff --git a/library/tests/args.cpp b/library/tests/args.cpp index 66ce180e..c59c259c 100644 --- a/library/tests/args.cpp +++ b/library/tests/args.cpp @@ -90,11 +90,11 @@ void usage( cout << "Usage: " << basename << " [options]\n\n" << "-f, --file s Input file, or the number n;\n" << - " '100' means hands/list100.txt under the current\n" << - " directory (or BUILD_WORKING_DIRECTORY /\n" << - " BUILD_WORKSPACE_DIRECTORY under bazel run), else\n" << - " relative to the dtest binary\n" << - " (bazel-bin/library/tests/).\n" << + " Relative paths (and '100' → hands/list100.txt) are\n" << + " resolved under the current directory, then under\n" << + " BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY\n" << + " (bazel run), else under the workspace root inferred\n" << + " from the dtest binary (bazel-bin/library/tests/).\n" << " (Default: input.txt)\n" << "\n" << "-s, --solver One of: solve, calc, play, par, dealerpar.\n" << @@ -309,6 +309,21 @@ string absolute_path_logical(const string& path) return normalize_logical_path((cwd / path).string()); } + +bool is_dtest_list_shorthand_arg(const string& arg) +{ + if (arg.empty()) + return false; + for (unsigned char c : arg) + { + if (c == '/' || c == '\\') + return false; + if (!std::isdigit(c)) + return false; + } + return true; +} + } // namespace @@ -324,50 +339,103 @@ string resolve_dtest_input_file( { if (path_exists(arg)) return arg; + if (is_absolute_path(arg)) + return string(); + // Windows drive-relative "C:foo" has a root-name; fs::path join may discard + // the BUILD_* / argv0 base. Only the literal path is attempted. + if (fs::path(arg).has_root_name()) + return string(); + + const bool use_list_shorthand = is_dtest_list_shorthand_arg(arg); + const string list_name = use_list_shorthand ? "list" + arg + ".txt" : string(); - const string list_name = "list" + arg + ".txt"; - // Keep generic separators so cwd hits match the documented hands/listN.txt form. - const string cwd_candidate = - (fs::path("hands") / list_name).generic_string(); - if (path_exists(cwd_candidate)) - return cwd_candidate; + if (use_list_shorthand) + { + // Keep generic separators so cwd hits match the documented hands/listN.txt form. + const string cwd_candidate = + (fs::path("hands") / list_name).generic_string(); + if (path_exists(cwd_candidate)) + return cwd_candidate; + } // bazel run moves CWD into the runfiles tree; it exports the invoke-time - // shell cwd and the workspace root so we can still find hands/. - auto from_env_dir = [&](const char* env_name) -> string + // shell cwd and the workspace root so relative -f paths still resolve. + auto from_env_dir = [&](const char* env_name, const fs::path& rel) -> string { + if (rel.has_root_name() || rel.has_root_directory()) + return string(); const char* dir = std::getenv(env_name); if (dir == nullptr || dir[0] == '\0') return string(); - const string candidate = normalize_logical_path( - (fs::path(dir) / "hands" / list_name).string()); + const string candidate = + normalize_logical_path((fs::path(dir) / rel).string()); if (path_exists(candidate)) return candidate; return string(); }; - if (const string found = from_env_dir("BUILD_WORKING_DIRECTORY"); !found.empty()) - return found; - if (const string found = from_env_dir("BUILD_WORKSPACE_DIRECTORY"); !found.empty()) - return found; - // Climb parents in the path *string* (do not use "/../" with filesystem // resolution — that follows a bazel-bin symlink into the execroot and misses // the workspace hands/ directory). bazel-bin/library/tests/dtest → four // parent_path steps to the repo root. - fs::path dir(absolute_path_logical(argv0)); - for (unsigned i = 0; i < 4; ++i) + auto workspace_root_from_argv0 = [&]() -> fs::path { - const fs::path parent = dir.parent_path(); - if (parent.empty()) - return string(); - dir = parent; + fs::path dir(absolute_path_logical(argv0)); + for (unsigned i = 0; i < 4; ++i) + { + const fs::path parent = dir.parent_path(); + if (parent.empty()) + return {}; + dir = parent; + } + return dir; + }; + + if (use_list_shorthand) + { + const fs::path list_rel = fs::path("hands") / list_name; + if (const string found = + from_env_dir("BUILD_WORKING_DIRECTORY", list_rel); !found.empty()) + { + return found; + } + if (const string found = + from_env_dir("BUILD_WORKSPACE_DIRECTORY", list_rel); !found.empty()) + { + return found; + } + } + + // Prefer list shorthand under bazel dirs / argv0 before a bare relative + // name (so -f 42 keeps resolving to hands/list42.txt even if a file named + // "42" exists at the workspace root). + if (const string found = + from_env_dir("BUILD_WORKING_DIRECTORY", arg); !found.empty()) + { + return found; } + if (const string found = + from_env_dir("BUILD_WORKSPACE_DIRECTORY", arg); !found.empty()) + { + return found; + } + + const fs::path root = workspace_root_from_argv0(); + if (root.empty()) + return string(); - const string bin_candidate = - normalize_logical_path((dir / "hands" / list_name).string()); - if (path_exists(bin_candidate)) - return bin_candidate; + if (use_list_shorthand) + { + const string bin_candidate = + normalize_logical_path((root / "hands" / list_name).string()); + if (path_exists(bin_candidate)) + return bin_candidate; + } + + const string bin_literal = + normalize_logical_path((root / arg).string()); + if (path_exists(bin_literal)) + return bin_literal; return string(); } @@ -431,10 +499,15 @@ void read_args( } cout << "Input file '" << optarg << "' not found\n"; - cout << "Also tried hands/list" << optarg << - ".txt under the current directory, " - "BUILD_WORKING_DIRECTORY, BUILD_WORKSPACE_DIRECTORY, " - "and relative to the dtest binary\n"; + // Absolute / drive-relative args only attempt the literal path. + if (!is_dtest_absolute_path(optarg) && + !fs::path(optarg).has_root_name()) + { + cout << "Also tried that path under the current directory, " + "BUILD_WORKING_DIRECTORY, BUILD_WORKSPACE_DIRECTORY, " + "and under the workspace root inferred from the dtest binary; " + "for numeric -f N, also hands/listN.txt\n"; + } nextToken -= 2; errFlag = true; break; diff --git a/library/tests/args.hpp b/library/tests/args.hpp index dde54022..5d8989b4 100644 --- a/library/tests/args.hpp +++ b/library/tests/args.hpp @@ -32,10 +32,14 @@ void print_options(); /// Resolve `-f` / `--file` to an existing regular file. /// -/// Order: (1) `arg` as a literal path; (2) `hands/list{arg}.txt` under the -/// current working directory; (3) the same under `BUILD_WORKING_DIRECTORY` / -/// `BUILD_WORKSPACE_DIRECTORY` (set by `bazel run`); (4) relative to the -/// directory of `argv0` (the usual `bazel-bin/library/tests/dtest` layout). +/// Tries, in order: the literal path under the current working directory; +/// for purely numeric `arg` (e.g. `"100"`), `hands/list{arg}.txt` under cwd; +/// that list form, then the literal relative path, +/// under `BUILD_WORKING_DIRECTORY` / `BUILD_WORKSPACE_DIRECTORY` (set by +/// `bazel run`); then those same two forms under the workspace root inferred +/// by climbing four parents from `argv0` (the usual +/// `bazel-bin/library/tests/dtest` layout). Absolute paths, and Windows +/// drive-relative forms like `C:foo`, only attempt the literal path as given. /// Directories are not accepted (avoids treating e.g. `-f hands` as a file). /// @return Resolved path, or empty if no regular file is found std::string resolve_dtest_input_file( diff --git a/library/tests/args_test.cpp b/library/tests/args_test.cpp index cb49c94f..9451aa28 100644 --- a/library/tests/args_test.cpp +++ b/library/tests/args_test.cpp @@ -3,7 +3,9 @@ #include +#include #include +#include #include #include #include @@ -264,6 +266,79 @@ TEST_F(HandsLayoutFixture, ResolveNumericUsesBazelWorkingDirectory) root_ + "hands/list42.txt")); } +TEST_F(HandsLayoutFixture, ResolveNumericPrefersListOverLiteralUnderBazelWorking) +{ + // A workspace-root file named "42" must not win over hands/list42.txt when + // resolving numeric -f under BUILD_WORKING_DIRECTORY. + { + std::ofstream out(root_ + "42"); + out << "literal-trap\n"; + } + + const std::string runfiles = + std::string(::testing::TempDir()) + "dtest_hands_runfiles_num_pref/"; + ASSERT_TRUE(make_dir(runfiles)); + ASSERT_EQ(change_dir(runfiles.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(root_.c_str()); + workspace.set(nullptr); + + EXPECT_TRUE(same_path( + resolve_dtest_input_file("42", "dtest"), + root_ + "hands/list42.txt")); +} + +TEST_F(HandsLayoutFixture, ResolvePathLikeArgDoesNotUseListShorthand) +{ + // A path-like -f must not probe nonsense list-shorthand candidates such as + // hands/listhands/list42.txt.txt when the literal path is missing. + const std::string trap = root_ + "hands/listhands/list42.txt.txt"; + ASSERT_TRUE(make_dir(root_ + "hands/listhands")); + { + std::ofstream out(trap); + out << "trap\n"; + } + { + std::error_code ec; + std::filesystem::remove(root_ + "hands/list42.txt", ec); + ASSERT_FALSE(ec) << ec.message(); + } + + const std::string runfiles = + std::string(::testing::TempDir()) + "dtest_hands_runfiles_no_shorthand/"; + ASSERT_TRUE(make_dir(runfiles)); + ASSERT_EQ(change_dir(runfiles.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(root_.c_str()); + workspace.set(nullptr); + + EXPECT_TRUE( + resolve_dtest_input_file("hands/list42.txt", "dtest").empty()); +} + +TEST_F(HandsLayoutFixture, ResolveLiteralRelativeUsesBazelWorkingDirectory) +{ + // bazelisk run //library/tests:dtest -- -f hands/list42.txt must find the + // path relative to the invoke-time shell cwd, not the runfiles tree. + const std::string runfiles = + std::string(::testing::TempDir()) + "dtest_hands_runfiles_lit/"; + ASSERT_TRUE(make_dir(runfiles)); + ASSERT_EQ(change_dir(runfiles.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(root_.c_str()); + workspace.set(nullptr); + + EXPECT_TRUE(same_path( + resolve_dtest_input_file("hands/list42.txt", "dtest"), + root_ + "hands/list42.txt")); +} + TEST_F(HandsLayoutFixture, ResolveNumericUsesBazelWorkspaceDirectory) { const std::string runfiles = @@ -281,6 +356,58 @@ TEST_F(HandsLayoutFixture, ResolveNumericUsesBazelWorkspaceDirectory) root_ + "hands/list42.txt")); } +TEST_F(HandsLayoutFixture, ResolveLiteralRelativeUsesBazelWorkspaceDirectory) +{ + const std::string runfiles = + std::string(::testing::TempDir()) + "dtest_hands_runfiles_ws_lit/"; + ASSERT_TRUE(make_dir(runfiles)); + ASSERT_EQ(change_dir(runfiles.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(nullptr); + workspace.set(root_.c_str()); + + EXPECT_TRUE(same_path( + resolve_dtest_input_file("hands/list42.txt", "dtest"), + root_ + "hands/list42.txt")); +} + +TEST_F(HandsLayoutFixture, ResolveLiteralRelativeFallsBackRelativeToBinary) +{ + // Same layout as numeric binary-relative lookup, but with an explicit path. + ASSERT_EQ(change_dir(original_cwd_.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(nullptr); + workspace.set(nullptr); + + EXPECT_TRUE(same_path( + resolve_dtest_input_file("hands/list42.txt", binary_path_), + root_ + "hands/list42.txt")); +} + +TEST_F(HandsLayoutFixture, ResolveNumericPrefersListOverLiteralRelativeToBinary) +{ + // Workspace-root file "42" must not shadow hands/list42.txt for numeric -f + // when falling back via argv0. + { + std::ofstream out(root_ + "42"); + out << "literal-trap\n"; + } + ASSERT_EQ(change_dir(original_cwd_.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(nullptr); + workspace.set(nullptr); + + EXPECT_TRUE(same_path( + resolve_dtest_input_file("42", binary_path_), + root_ + "hands/list42.txt")); +} + TEST_F(HandsLayoutFixture, ResolveNumericWithRelativeArgv0FromOtherCwd) { // Mimic running `../bazel-bin/library/tests/dtest -f 42` from a sibling of @@ -331,6 +458,81 @@ TEST_F(HandsLayoutFixture, ResolveRejectsDirectoryAsLiteralPath) EXPECT_TRUE(resolve_dtest_input_file(root_ + "hands", "dtest").empty()); } +TEST_F(HandsLayoutFixture, ResolveAbsoluteMissingDoesNotUseListShorthand) +{ + // A missing absolute -f must not fall through to hands/list{arg}.txt + // (list shorthand concatenates the absolute path into a nested name). + ASSERT_EQ(change_dir(original_cwd_.c_str()), 0); + + // Pick a unique absolute path that is not present on this machine. + const std::string token = + "no_such_dtest_abs_" + + std::to_string(static_cast( + reinterpret_cast(this))); +#ifdef _WIN32 + const std::string missing_abs = "\\" + token; +#else + const std::string missing_abs = "/" + token; +#endif + ASSERT_FALSE(std::filesystem::exists(missing_abs)); + + // Trap file at the binary-relative list-shorthand location that a buggy + // fallthrough would incorrectly accept. + const std::filesystem::path trap = + std::filesystem::path(root_) / "hands" / + ("list" + missing_abs + ".txt"); + { + std::error_code ec; + std::filesystem::create_directories(trap.parent_path(), ec); + ASSERT_FALSE(ec) << ec.message(); + } + { + std::ofstream out(trap); + ASSERT_TRUE(out) << trap.string(); + out << "trap\n"; + } + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(nullptr); + workspace.set(nullptr); + + EXPECT_TRUE( + resolve_dtest_input_file(missing_abs, binary_path_).empty()); +} + +#ifdef _WIN32 +TEST_F(HandsLayoutFixture, ResolveDriveRelativeMissingDoesNotJoinUnderBazelDirs) +{ + // Drive-relative "X:foo" is not absolute, but fs::path join can discard the + // BUILD_* base. Missing drive-relative -f must not resolve via env joins. + ASSERT_GE(root_.size(), 2u); + ASSERT_EQ(root_[1], ':'); + + const std::string token = + "no_such_dtest_drive_rel_" + + std::to_string(static_cast( + reinterpret_cast(this))); + const std::string missing_drive_rel = + std::string(1, root_[0]) + ":" + token; + ASSERT_FALSE(is_dtest_absolute_path(missing_drive_rel)); + ASSERT_FALSE(std::filesystem::exists(missing_drive_rel)); + + const std::string runfiles = + std::string(::testing::TempDir()) + "dtest_hands_drive_rel/"; + ASSERT_TRUE(make_dir(runfiles)); + ASSERT_EQ(change_dir(runfiles.c_str()), 0); + + const EnvVarGuard working("BUILD_WORKING_DIRECTORY"); + const EnvVarGuard workspace("BUILD_WORKSPACE_DIRECTORY"); + working.set(root_.c_str()); + workspace.set(root_.c_str()); + + EXPECT_TRUE( + resolve_dtest_input_file(missing_drive_rel, binary_path_).empty()); +} +#endif + TEST(Args, AbsolutePathDetection) { EXPECT_FALSE(is_dtest_absolute_path("tmp/dtest"));