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.

Sign up for GitHub

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

Jump to bottom

Ensure absolute path for include #2092

Merged
merged 1 commit into from Mar 9, 2022
Merged

Ensure absolute path for include #2092

merged 1 commit into from Mar 9, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 9, 2022

Ensure the path of file to include in instance_t::include_directive
is always an absolute path.

Previously when the journal file is given through stdin, we prepend a
"./" to the filename to include. However, in Boost >= 1.77,
path::normalize strips the leading "./" [1]. Our resolve_path
function calls normalize and thus now it returns "file" for "./file"
instead of the previous "./file".

This change causes a failing test regress/BF3C1F82-2 [2], and also
breaks the include directive for stdin input:

$ touch file-to-include
$ echo "include file-to-include" | ledger -f - reg

gives

While parsing file "", line 1:
Error: File to include was not found: "file-to-include"

Therefore, we change to prepend the context.current_directory to
make the filename absolute in this case as well. The test
regress/BF3C1F82-2 is also updated to match the new output.

Fixes #2075.

[1] boostorg/filesystem@16bd89b
[2] #2075

Ensure absolute path for include
1cb9b84 
Ensure the path of file to include in `instance_t::include_directive`
is always an absolute path.

Previously when the journal file is given through stdin, we prepend a
"./" to the filename to include. However, in Boost >= 1.77,
`path::normalize` strips the leading "./" [1]. Our `resolve_path`
function calls `normalize` and thus now it returns "file" for "./file"
instead of the previous "./file".

This change causes a failing test regress/BF3C1F82-2 [2], and also
breaks the `include` directive for stdin input:

    $ touch file-to-include
    $ echo "include file-to-include" | ledger -f - reg

gives

    While parsing file "", line 1:
    Error: File to include was not found: "file-to-include"

Therefore, we change to prepend the `context.current_directory` to
make the filename absolute in this case as well. The test
regress/BF3C1F82-2 is also updated to match the new output.

Fixes #2075.

[1] boostorg/filesystem@16bd89b
[2] #2075
@ghost
Copy link
Author

ghost commented Mar 9, 2022

By the way, I briefly checked and there seems to be no breakage due to the Boost change in the two other places that call resolve_path.

Thanks!

@tbm
Copy link
Contributor

tbm commented Mar 9, 2022

@kunhtkun thanks so much for submitting a fix for this issue! That's one of the stoppers for getting a new release out.

I'm not famliar with the code but why do we have to prepand a directory at all?

Would just

filename = / line;

work?

I'm sure there's a reason this doesn't work. Just asking.

@tbm tbm requested a review from jwiegley March 9, 2022 06:55
@ghost
Copy link
Author

ghost commented Mar 9, 2022 •
edited by ghost
Loading

Hi @tbm! You are welcome and thanks for taking care of this awesome project!

Regarding the prepended path, we actually requires an existent parent path:

ledger/src/textual.cc

Lines 758 to 767 in 474d2b8

mask_t glob;
path parent_path = filename.parent_path();
glob.assign_glob('^' + filename.filename().string() + '$');
bool files_found = false;
if (exists(parent_path)) {
filesystem::directory_iterator end;
// Sort parent_path since on some file systems it is unsorted.
std::vector<path> sorted_parent_path;

otherwise include_directive will not try to find the file at all. That is also why adding "./" fails after Boost 1.77 -- now it strips the leading "./" which is the parent path.

Honestly the snippet linked above is a bit more complicated than I would imagine, but I guess there is a reason to do so. (After some sleep I realized it is complicated because it supports globs.)

tbm
tbm approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@tbm tbm left a comment

Choose a reason for hiding this comment

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

I've run some tests and I think this works as expected.

Kunht, thanks so much for this fix. This issue has been outstanding for far too long.

If you have time to fix other issues, please submit PRs and John and I will review. Ledger needs help.

@tbm tbm merged commit ba8ec32 into ledger:master Mar 9, 2022
@ghost ghost mentioned this pull request Mar 12, 2022
Merged
@purcell purcell mentioned this pull request Mar 24, 2022
Closed
@bcc32 bcc32 mentioned this pull request Dec 24, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwiegley jwiegley Awaiting requested review from jwiegley

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test failure with Boost 1.77.0 (regress/BF3C1F82-2.test)
1 participant
@tbm