Bug 225362 – Port Filesystem::fileMetadata() & Filesystem::getFileModificationTime() to std::filesystem

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Port Filesystem::getFileModificationTime() to std::filesystem.

[Chris Dumez]

Created attachment 427686 [details]
Patch

[Chris Dumez]

Created attachment 427687 [details]
Patch

[Chris Dumez]

Comment on attachment 427687 [details]
Patch

Looks like the new test is failing for the GTK port. Will try and figure it out.

[Chris Dumez]

Created attachment 427690 [details]
Patch

[Chris Dumez]

Created attachment 427691 [details]
Patch

[Chris Dumez]

Test is still failing in GTK:
../../Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp:560
Expected: (newModificationTime->secondsSinceEpoch().value()) > (timeBeforeModification.secondsSinceEpoch().value()), actual: -4.81751e+09 vs 1.62016e+09

Look like I am getting a negative modification time :/

[Chris Dumez]

Created attachment 427694 [details]
Patch

[Chris Dumez]

Created attachment 427712 [details]
Patch

[Darin Adler]

Comment on attachment 427694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427694&action=review

> Source/WTF/wtf/FileSystem.cpp:633
> +    auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now()));

There must be a way that doesn’t depend on two subsequent calls to now!

I looked here https://stackoverflow.com/questions/61030383/how-to-convert-stdfilesystemfile-time-type-to-time-t

Didn’t find something better.

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 427694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427694&action=review
> 
> > Source/WTF/wtf/FileSystem.cpp:633
> > +    auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now()));
> 
> There must be a way that doesn’t depend on two subsequent calls to now!
> 
> I looked here
> https://stackoverflow.com/questions/61030383/how-to-convert-
> stdfilesystemfile-time-type-to-time-t
> 
> Didn’t find something better.

I did some research and apparently there is no good way with C++17. This is the best I could find so I did this and added a FIXME comment for when we switch to C++20 (which has a better way).

[Darin Adler]

Comment on attachment 427712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427712&action=review

> Source/WTF/wtf/FileSystem.h:120
> -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path);
> -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadataFollowingSymlinks(const String& path);
> +WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path, ShouldFollowSymbolicLinks = ShouldFollowSymbolicLinks::No);

Is this an improvement?

[Chris Dumez]

(In reply to Darin Adler from comment #11)
> Comment on attachment 427712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427712&action=review
> 
> > Source/WTF/wtf/FileSystem.h:120
> > -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path);
> > -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadataFollowingSymlinks(const String& path);
> > +WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path, ShouldFollowSymbolicLinks = ShouldFollowSymbolicLinks::No);
> 
> Is this an improvement?

I think so. We already have the ShouldFollowSymbolicLinks enum and use it for other FileSystem functions. I don't see why fileMetadata() needs a separate function for this. I hesitated making the parameter mandatory though.

You seem to disagree?

[Darin Adler]

(In reply to Chris Dumez from comment #12)
> You seem to disagree?

I generally prefer named functions to parameters that set policy for a function, except in cases where the combinatorial explosion makes them impractical, or when it’s more common to pass an argument through from one level to the next, rather than pass a constant.

Just seems less wordy and more human.

I think I inherited that preference from Cocoa naming style.

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> (In reply to Chris Dumez from comment #12)
> > You seem to disagree?
> 
> I generally prefer named functions to parameters that set policy for a
> function, except in cases where the combinatorial explosion makes them
> impractical, or when it’s more common to pass an argument through from one
> level to the next, rather than pass a constant.
> 
> Just seems less wordy and more human.
> 
> I think I inherited that preference from Cocoa naming style.

Ok. I will revert this part of my change since it seems a little controversial. I do think that we should make our FileSystem API consistent eventually though, in one direction or another (I have a slight preference for the parameter but I won't fight you on this).

[Darin Adler]

(In reply to Chris Dumez from comment #14)
> I do think that we should make our FileSystem API consistent
> eventually though, in one direction or another (I have a slight preference
> for the parameter but I won't fight you on this).

I agree.

And my preference is also slight!

[Darin Adler]

Another strength of having a separate named function is that it’s easier to find out if it’s used or not.

[Chris Dumez]

Created attachment 427722 [details]
Patch

[Chris Dumez]

Created attachment 427729 [details]
Patch

[Chris Dumez]

Created attachment 427771 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> (In reply to Darin Adler from comment #9)
> > Comment on attachment 427694 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427694&action=review
> > 
> > > Source/WTF/wtf/FileSystem.cpp:633
> > > +    auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now()));
> > 
> > There must be a way that doesn’t depend on two subsequent calls to now!
> > 
> > I looked here
> > https://stackoverflow.com/questions/61030383/how-to-convert-
> > stdfilesystemfile-time-type-to-time-t
> > 
> > Didn’t find something better.
> 
> I did some research and apparently there is no good way with C++17. This is
> the best I could find so I did this and added a FIXME comment for when we
> switch to C++20 (which has a better way).

In this latest iteration, I use SFINAE to use std::filesystem::file_time_type::clock::to_time_t() whenever available (It is available on my machine but according to [1] is not available in MSVC and GCC 9).

[1] https://en.cppreference.com/w/cpp/filesystem/file_time_type

[Chris Dumez]

(In reply to Chris Dumez from comment #20)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Darin Adler from comment #9)
> > > Comment on attachment 427694 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=427694&action=review
> > > 
> > > > Source/WTF/wtf/FileSystem.cpp:633
> > > > +    auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now()));
> > > 
> > > There must be a way that doesn’t depend on two subsequent calls to now!
> > > 
> > > I looked here
> > > https://stackoverflow.com/questions/61030383/how-to-convert-
> > > stdfilesystemfile-time-type-to-time-t
> > > 
> > > Didn’t find something better.
> > 
> > I did some research and apparently there is no good way with C++17. This is
> > the best I could find so I did this and added a FIXME comment for when we
> > switch to C++20 (which has a better way).
> 
> In this latest iteration, I use SFINAE to use
> std::filesystem::file_time_type::clock::to_time_t() whenever available (It
> is available on my machine but according to [1] is not available in MSVC and
> GCC 9).
> 
> [1] https://en.cppreference.com/w/cpp/filesystem/file_time_type

But of course, it doesn't build on GTK :face-palm:

[Chris Dumez]

Created attachment 427773 [details]
Patch

[Chris Dumez]

Created attachment 427774 [details]
Patch

[Darin Adler]

Comment on attachment 427774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427774&action=review

> Source/WTF/wtf/FileSystem.cpp:671
> +    if (shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes && std::filesystem::is_symlink(fsPath, ec)) {
> +        fsPath = std::filesystem::read_symlink(fsPath, ec);
> +        if (ec)
> +            return WTF::nullopt;
> +    }

This follows one level of symbolic link. To "follow" a symbolic ink I think we may need to do this in a loop, likely with a limit of some sort to deal with cycles.

[Chris Dumez]

(In reply to Darin Adler from comment #24)
> Comment on attachment 427774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427774&action=review
> 
> > Source/WTF/wtf/FileSystem.cpp:671
> > +    if (shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes && std::filesystem::is_symlink(fsPath, ec)) {
> > +        fsPath = std::filesystem::read_symlink(fsPath, ec);
> > +        if (ec)
> > +            return WTF::nullopt;
> > +    }
> 
> This follows one level of symbolic link. To "follow" a symbolic ink I think
> we may need to do this in a loop, likely with a limit of some sort to deal
> with cycles.

Interesting. I did not realize this was a thing. I'll write a test and tweak the code accordingly.

[Chris Dumez]

Created attachment 427781 [details]
Patch

[Chris Dumez]

Anybody willing to review this? I have addressed all the feedback so far.

[Darin Adler]

Comment on attachment 427781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427781&action=review

> Source/WTF/wtf/FileSystem.cpp:130
> +    std::time_t timeT = toTimeT(fileTime);
> +    return WallTime::fromRawSeconds(timeT);

Don’t think we need the local variable.

> Source/WTF/wtf/FileSystem.cpp:133
> +static std::filesystem::path resolveSymlinks(std::filesystem::path path, std::error_code ec)

I think we intend that ec is an in/out argument, but here it’s only being passed in. I would expect tests to fail because of this, since failure would not be propagated to the caller.

> Source/WTF/wtf/FileSystem.cpp:136
> +    static const unsigned maxSymlinkDepth = MAXSYMLINKS;

constexpr

> Source/WTF/wtf/FileSystem.cpp:138
> +    static const unsigned maxSymlinkDepth = 40;

constexpr

[Darin Adler]

Started reviewing yesterday and spotted one significant mistake that makes me worry about the thoroughness of our testing. Don’t have time to finish reviewing right now, but sent you my comment in progress that include that mistake.

I applaud the goal of making this more elegant and less platform-specific, but I am worried about making a lot of changes here with room for error, without extensive testing, and without obvious compelling, concrete benefit that would cause us to take the risk. That’s why I am not super-fast to review, since I feel we need to be cautious.

[Chris Dumez]

(In reply to Darin Adler from comment #29)
> Started reviewing yesterday and spotted one significant mistake that makes
> me worry about the thoroughness of our testing. Don’t have time to finish
> reviewing right now, but sent you my comment in progress that include that
> mistake.
> 
> I applaud the goal of making this more elegant and less platform-specific,
> but I am worried about making a lot of changes here with room for error,
> without extensive testing, and without obvious compelling, concrete benefit
> that would cause us to take the risk. That’s why I am not super-fast to
> review, since I feel we need to be cautious.

I understand. I would just like to point out that we had almost no testing for our FileSystem API and my patches do bring in API tests. Clearly my tests were not enough since you've spotted mistakes but I have added tests for every mistake you spotted.

[Chris Dumez]

(In reply to Darin Adler from comment #28)
> Comment on attachment 427781 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427781&action=review
> 
> > Source/WTF/wtf/FileSystem.cpp:130
> > +    std::time_t timeT = toTimeT(fileTime);
> > +    return WallTime::fromRawSeconds(timeT);
> 
> Don’t think we need the local variable.

Will fix.

> 
> > Source/WTF/wtf/FileSystem.cpp:133
> > +static std::filesystem::path resolveSymlinks(std::filesystem::path path, std::error_code ec)
> 
> I think we intend that ec is an in/out argument, but here it’s only being
> passed in. I would expect tests to fail because of this, since failure would
> not be propagated to the caller.

The function also returns a default-constructed path when failing so the caller (even though it didn't have the right error_code) would still fail later on.

> 
> > Source/WTF/wtf/FileSystem.cpp:136
> > +    static const unsigned maxSymlinkDepth = MAXSYMLINKS;
> 
> constexpr
> 
> > Source/WTF/wtf/FileSystem.cpp:138
> > +    static const unsigned maxSymlinkDepth = 40;
> 
> constexpr

Will fix.

[Darin Adler]

Yes, really happy about the new tests you are adding! If someone else doesn’t review, I will eventually get to it.

[Chris Dumez]

Created attachment 427916 [details]
Patch

[Darin Adler]

Comment on attachment 427916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427916&action=review

> LayoutTests/ChangeLog:8
> +        Stop testing the size of a directory in the data transfer API as the result is implementation-specific.

I understand that this test wasn’t a good way to test the value of size. But is it right to just stop testing it at all? How can we best make sure it keeps working properly?

[Chris Dumez]

(In reply to Darin Adler from comment #34)
> Comment on attachment 427916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427916&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Stop testing the size of a directory in the data transfer API as the result is implementation-specific.
> 
> I understand that this test wasn’t a good way to test the value of size. But
> is it right to just stop testing it at all? How can we best make sure it
> keeps working properly?

Maybe I can just add 0 to the list of allowed values then.

[Chris Dumez]

Created attachment 427958 [details]
Patch

[EWS]

Found 1 new test failure: http/tests/media/clearkey/collect-webkit-media-session.html

[EWS]

Committed r277158 (237444@main): <https://commits.webkit.org/237444@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427958 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77640765>