Skip to content

Commit

Permalink
Fix absolute/relative path handling in --affected
Browse files Browse the repository at this point in the history
The conditional was incorrectly reversed due to my misreading of
`std::error_code` so we never attempted to look up the absolute or
relative path for paths passed in to `--affected`.

It was a bit tricky to do unit test for absolute paths as they need to
be generated beforehand.  This commit is less lazy and includes unit
tests for absolute and relative path handling by generating files in the
CMake generation step.
  • Loading branch information
elliotgoodrich committed Oct 13, 2024
1 parent 775d78b commit f3a3d3e
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 6 deletions.
11 changes: 11 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,17 @@ set(CPACK_NSIS_MODIFY_PATH ON)
set(CPACK_NSIS_IGNORE_LICENSE_PAGE ON)
include(CPack)

# Generate test files for the absolute/relative unit test
get_filename_component(ABSOLUTE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/tests/absolute/relative" ABSOLUTE)
string(REPLACE ":" "$:" ABSOLUTE_PATH_ESCAPED "${ABSOLUTE_PATH}")
file(WRITE "${CMAKE_CURRENT_SOURCE_DIR}/tests/absolute/changed.txt" "${ABSOLUTE_PATH}\nabsolute")
string(APPEND NINJA_CONTENTS "rule copy\n")
string(APPEND NINJA_CONTENTS " command = ninja --version \$in -> \$out\n")
string(APPEND NINJA_CONTENTS "build out_relative: copy absolute\n")
string(APPEND NINJA_CONTENTS "build out_absolute: copy ${ABSOLUTE_PATH_ESCAPED}\n")
file(WRITE "${CMAKE_CURRENT_SOURCE_DIR}/tests/absolute/build.ninja" "${NINJA_CONTENTS}")
file(COPY_FILE "${CMAKE_CURRENT_SOURCE_DIR}/tests/absolute/build.ninja" "${CMAKE_CURRENT_SOURCE_DIR}/tests/absolute/expected.ninja")

file(
GLOB TRIMJA_TESTS
LIST_DIRECTORIES true
Expand Down
12 changes: 6 additions & 6 deletions src/trimutil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -668,10 +668,10 @@ void TrimUtil::trim(std::ostream& output,
// If that does not indicate a path, try the absolute path
std::filesystem::path p(line);
if (!p.is_absolute()) {
std::error_code okay;
std::error_code error;
const std::filesystem::path& absolute =
attempted.emplace_back(std::filesystem::absolute(p, okay));
if (okay) {
attempted.emplace_back(std::filesystem::absolute(p, error));
if (!error) {
std::string absoluteStr = absolute.string();
const std::optional<std::size_t> index = graph.findPath(absoluteStr);
if (index.has_value()) {
Expand All @@ -689,10 +689,10 @@ void TrimUtil::trim(std::ostream& output,
// If neither indicates a path, then try the path relative to the ninja
// file
if (!p.is_relative()) {
std::error_code okay;
std::error_code error;
const std::filesystem::path& relative =
attempted.emplace_back(std::filesystem::relative(p, okay));
if (okay) {
attempted.emplace_back(std::filesystem::relative(p, error));
if (!error) {
std::string relativeStr = relative.string();
const std::optional<std::size_t> index = graph.findPath(relativeStr);
if (index.has_value()) {
Expand Down
4 changes: 4 additions & 0 deletions tests/absolute/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# These files are generated at configure time
changed.txt
build.ninja
expected.ninja
Empty file added tests/absolute/absolute
Empty file.
1 change: 1 addition & 0 deletions tests/absolute/bootstrap.ninja
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include build.ninja
Empty file added tests/absolute/relative
Empty file.

0 comments on commit f3a3d3e

Please sign in to comment.