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

temp_directory_path() returns path with trailing slash #44652

Closed
ldionne opened this issue Mar 25, 2020 · 4 comments
Closed

temp_directory_path() returns path with trailing slash #44652

ldionne opened this issue Mar 25, 2020 · 4 comments
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. wontfix Issue is real, but we can't or won't fix it. Not invalid

Comments

@ldionne
Copy link
Member

ldionne commented Mar 25, 2020

Bugzilla Link 45307
Resolution WONTFIX
Resolved on May 20, 2020 14:17
Version unspecified
OS All
CC @mclow

Extended Description

The following program works when run in an empty environment, but not when TEMPDIR or some other temporary-directory-pointing environment variable is defined:

$ cat <<EOF | clang++ -xc++ -std=c++17 - && ./a.out
#include <filesystem>
#include <iostream>
namespace fs = std::filesystem;
int main() {
    fs::path tmp = fs::temp_directory_path();
    fs::path p = tmp / "hello";
    std::cout << "tmp = " << tmp << std::endl;
    std::cout << "p.parent_path() = " << p.parent_path() << std::endl;
    assert(p.parent_path() == tmp);
}
EOF

When run as ./a.out, the program fails:

tmp = "/var/folders/10/r6bw68bs5b9gwjtrnl9dz0vm0000gn/T/"
p.parent_path() = "/var/folders/10/r6bw68bs5b9gwjtrnl9dz0vm0000gn/T"
Assertion failed: (p.parent_path() == tmp), function main, file <stdin>, line 9.

Unsurprisingly, the environment contains the following entry:

TMPDIR=/var/folders/10/r6bw68bs5b9gwjtrnl9dz0vm0000gn/T/

When run as env -i ./a.out, the program succeeds and prints the following:

tmp = "/tmp"
p.parent_path() = "/tmp"

Comparison of paths is based on comparing the file-names exposed by begin() through end(), and that is specified to yield an empty element at the end if the path ends with a directory separator. So the comparison appears to be behaving correctly.

However, I believe temp_directory_path() should normalize its return value and not return a path with a trailing slash if the path it picks up from the environment has a trailing slash. The reason is that the Standard says (http://eel.is/c++draft/fs.op.temp.dir.path#5):

Example: For POSIX-based operating systems, an implementation might return the path supplied by the first environment variable found in the list TMPDIR, TMP, TEMP, TEMPDIR, or if none of these are found, "/tmp".

It seems weird that we'd return a path with a trailing slash when found in an environment variable, but just "/tmp" without a trailing slash otherwise.

@ldionne
Copy link
Member Author

ldionne commented Mar 25, 2020

Potential fix under review: https://reviews.llvm.org/D76798

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 26, 2020

I don't necessarily agree.

First, no implementation currently has the behavior you describe.
Both libstdc++ and Boost filesystem return the environment variable as specified.

Also, on OS X, the TEMPDIR variable includes the trailing slash. If that's what the operating system does, why should we differ?

I also think there are some subtle differences in pathname resolution. Specifically around symlinks and when the basename doesn't exist.

For example, let's assume TEMPDIR is /this-directory-does-not-exist/. touch foo; cp foo $TEMPDIR fails because there is no such directory. Without the trailing slash it succeeds and copies foo to a new file called this-directory-does-not-exist.

In general trailing slashes are used to denote that the entity is a directory. I don't see a strong reason to differ.

But if you disagree, I'm willing to proceed with your fix, on the condition that you file a LWG issue about this proposing this behavior as standard.

@ldionne
Copy link
Member Author

ldionne commented Mar 26, 2020

Okay, I can see myself agreeing with you, however do you think in this case for consistency the Standard should change the note in http://eel.is/c++draft/fs.op.temp.dir.path#5 to:

Example: For POSIX-based operating systems, an implementation might return the path supplied by the first environment variable found in the list TMPDIR, TMP, TEMP, TEMPDIR, or if none of these are found, "/tmp/"

(notice I added a trailing slash to "/tmp/")

Do you think that makes sense? We could change our implementation accordingly.

@ldionne
Copy link
Member Author

ldionne commented May 20, 2020

Revisiting this, I think returning the environment variable as-is is fine. In the test program, we should canonicalize the result of fs::temp_directory_path() if we want to check the assertion we're checking.

I'd also argue that path{"/tmp"} and path{"/tmp/"} should probably compare equal, but that's a DR if anything.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@Quuxplusone Quuxplusone added the wontfix Issue is real, but we can't or won't fix it. Not invalid label Jan 20, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. wontfix Issue is real, but we can't or won't fix it. Not invalid
Projects
None yet
Development

No branches or pull requests

3 participants