LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 45307 - temp_directory_path() returns path with trailing slash
Summary: temp_directory_path() returns path with trailing slash
Status: RESOLVED WONTFIX
Alias: None
Product: libc++
Classification: Unclassified
Component: All Bugs (show other bugs)
Version: unspecified
Hardware: PC All
: P enhancement
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-25 12:27 PDT by Louis Dionne
Modified: 2020-05-20 14:17 PDT (History)
3 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Louis Dionne 2020-03-25 12:27:22 PDT
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.
Comment 1 Louis Dionne 2020-03-25 13:31:36 PDT
Potential fix under review: https://reviews.llvm.org/D76798
Comment 2 Eric Fiselier 2020-03-26 09:01:53 PDT
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.
Comment 3 Louis Dionne 2020-03-26 09:23:33 PDT
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.
Comment 4 Louis Dionne 2020-05-20 14:17:12 PDT
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.