Bug 225362 - Port Filesystem::fileMetadata() & Filesystem::getFileModificationTime() to std::filesystem
Summary: Port Filesystem::fileMetadata() & Filesystem::getFileModificationTime() to st...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-04 10:48 PDT by Chris Dumez
Modified: 2021-05-07 02:49 PDT (History)
13 users (show)

See Also:


Attachments
Patch (6.47 KB, patch)
2021-05-04 10:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2021-05-04 10:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.62 KB, patch)
2021-05-04 11:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2021-05-04 12:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.92 KB, patch)
2021-05-04 13:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.23 KB, patch)
2021-05-04 16:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (23.22 KB, patch)
2021-05-04 18:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.58 KB, patch)
2021-05-04 20:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.89 KB, patch)
2021-05-05 09:11 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.20 KB, patch)
2021-05-05 09:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (28.14 KB, patch)
2021-05-05 09:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.88 KB, patch)
2021-05-05 10:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.99 KB, patch)
2021-05-06 11:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (31.14 KB, patch)
2021-05-06 16:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-05-04 10:48:34 PDT
Port Filesystem::getFileModificationTime() to std::filesystem.
Comment 1 Chris Dumez 2021-05-04 10:49:49 PDT
Created attachment 427686 [details]
Patch
Comment 2 Chris Dumez 2021-05-04 10:55:21 PDT
Created attachment 427687 [details]
Patch
Comment 3 Chris Dumez 2021-05-04 11:52:27 PDT
Comment on attachment 427687 [details]
Patch

Looks like the new test is failing for the GTK port. Will try and figure it out.
Comment 4 Chris Dumez 2021-05-04 11:57:53 PDT
Created attachment 427690 [details]
Patch
Comment 5 Chris Dumez 2021-05-04 12:04:48 PDT
Created attachment 427691 [details]
Patch
Comment 6 Chris Dumez 2021-05-04 13:00:31 PDT
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 :/
Comment 7 Chris Dumez 2021-05-04 13:29:53 PDT
Created attachment 427694 [details]
Patch
Comment 8 Chris Dumez 2021-05-04 16:58:35 PDT
Created attachment 427712 [details]
Patch
Comment 9 Darin Adler 2021-05-04 17:16:15 PDT
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.
Comment 10 Chris Dumez 2021-05-04 17:27:34 PDT
(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).
Comment 11 Darin Adler 2021-05-04 17:45:44 PDT
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?
Comment 12 Chris Dumez 2021-05-04 18:00:36 PDT
(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?
Comment 13 Darin Adler 2021-05-04 18:12:47 PDT
(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.
Comment 14 Chris Dumez 2021-05-04 18:22:55 PDT
(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).
Comment 15 Darin Adler 2021-05-04 18:31:07 PDT
(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!
Comment 16 Darin Adler 2021-05-04 18:31:52 PDT
Another strength of having a separate named function is that it’s easier to find out if it’s used or not.
Comment 17 Chris Dumez 2021-05-04 18:41:32 PDT
Created attachment 427722 [details]
Patch
Comment 18 Chris Dumez 2021-05-04 20:53:22 PDT
Created attachment 427729 [details]
Patch
Comment 19 Chris Dumez 2021-05-05 09:11:40 PDT
Created attachment 427771 [details]
Patch
Comment 20 Chris Dumez 2021-05-05 09:14:22 PDT
(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
Comment 21 Chris Dumez 2021-05-05 09:15:06 PDT
(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:
Comment 22 Chris Dumez 2021-05-05 09:25:51 PDT
Created attachment 427773 [details]
Patch
Comment 23 Chris Dumez 2021-05-05 09:36:15 PDT
Created attachment 427774 [details]
Patch
Comment 24 Darin Adler 2021-05-05 10:03:57 PDT
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.
Comment 25 Chris Dumez 2021-05-05 10:07:42 PDT
(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.
Comment 26 Chris Dumez 2021-05-05 10:56:58 PDT
Created attachment 427781 [details]
Patch
Comment 27 Chris Dumez 2021-05-06 10:31:03 PDT
Anybody willing to review this? I have addressed all the feedback so far.
Comment 28 Darin Adler 2021-05-06 11:02:41 PDT
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
Comment 29 Darin Adler 2021-05-06 11:04:17 PDT
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.
Comment 30 Chris Dumez 2021-05-06 11:24:03 PDT
(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.
Comment 31 Chris Dumez 2021-05-06 11:39:58 PDT
(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.
Comment 32 Darin Adler 2021-05-06 11:46:43 PDT
Yes, really happy about the new tests you are adding! If someone else doesn’t review, I will eventually get to it.
Comment 33 Chris Dumez 2021-05-06 11:56:20 PDT
Created attachment 427916 [details]
Patch
Comment 34 Darin Adler 2021-05-06 16:23:53 PDT
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?
Comment 35 Chris Dumez 2021-05-06 16:27:32 PDT
(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.
Comment 36 Chris Dumez 2021-05-06 16:44:44 PDT
Created attachment 427958 [details]
Patch
Comment 37 EWS 2021-05-06 21:09:31 PDT
Found 1 new test failure: http/tests/media/clearkey/collect-webkit-media-session.html
Comment 38 EWS 2021-05-06 22:00:47 PDT
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].
Comment 39 Radar WebKit Bug Importer 2021-05-06 22:01:16 PDT
<rdar://problem/77640765>