WIP: Use std::filesystem as standard #11564

ghost · 2024-08-08T14:50:17Z

Deprecate path manipulations and use std::filesystem as this is introduced in C++17

In reference: #11443

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

The PR has a description, explaining both the problem and the solution.
The description mentions which forms of testing were done and the testing seems reasonable.
Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

flow/Platform.actor.cpp

flow/include/flow/Platform.h

giorgiozoppi

do we have passing tests about this? I added mroe comments.

ghost · 2024-08-09T14:38:36Z

do we have passing tests about this? I added mroe comments.

Thank you so much for your feedback! I'm reviewing it now. I don't think there's passing tests yet but will check as soon as I can.

ghost · 2024-08-12T12:05:56Z

do we have passing tests about this? I added mroe comments.

Sorry for late update, no passing tests and there are quite a few things that are failing / not building. Going to spend some time refactoring code to align with filesystem.

foundationdb-ci · 2024-08-12T16:29:27Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: e723c46
Duration 0:03:47
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-12T16:29:56Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: e723c46
Duration 0:04:16
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-12T16:29:58Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: e723c46
Duration 0:04:17
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2024-08-12T16:29:58Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: e723c46
Duration 0:04:16
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-12T16:30:01Z

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: e723c46
Duration 0:04:20
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-12T16:30:50Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: e723c46
Duration 0:05:11
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-12T16:31:31Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: e723c46
Duration 0:05:52
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

giorgiozoppi · 2024-08-14T21:31:29Z

flow/Platform.actor.cpp

@@ -2324,7 +2318,7 @@ void atomicReplace(std::filesystem::path const& path, std::string const& content
 		INJECT_FAULT(io_error, "atomicReplace"); // atomic rename failed

 		std::string tempfilename =


which version is set, if we support C++20 upwards this is a good case for std::format. Since we're creating at least 2 tmp strings.

I believe this is for C++17 but if maintainers can verify this that can be helpful, std::format would be the better way to go about this

giorgiozoppi · 2024-08-14T21:32:42Z

flow/Platform.actor.cpp

@@ -2648,7 +2641,7 @@ ACTOR Future<std::vector<std::string>> findFiles(std::string directory,
 			}
 			std::string name(dit->d_name);
 			struct stat buf;
-			if (stat(joinPath(directory, name).c_str(), &buf)) {
+			if (stat(directory / name, &buf)) {


beautiful simplicity....great.

giorgiozoppi · 2024-08-14T21:36:12Z

flow/Platform.actor.cpp

@@ -4104,53 +4097,53 @@ void platformSpecificDirectoryOpsTests(const std::string& cwd, int& errors) {
 	ASSERT(symlink("../backups/four", "simfdb/backups/five") == 0);

 	errors += testPathFunction2(


this is complex, same function with differente params, better put the params in an approriate data structure and iterate, doing append but it is out of scope for this pr.

I agree, I think that this would be best. I was hoping to correct the std::filesystem but there's still a lot to go through then go back to refactor this as a separate PR. Thank you for the heads up, I'll note this down for future PR to refactor this

giorgiozoppi · 2024-08-14T21:38:29Z

flow/Platform.actor.cpp

 	errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three", true, true, platform_error());
 	errors += testPathFunction2(
-	    "abspath", abspath, "simfdb/backups/five/../three", true, false, joinPath(cwd, "simfdb/backups/one/three"));
+	    "abspath", abspath, "simfdb/backups/five/../three", true, false, cwd / "simfdb/backups/one/three");
 	errors += testPathFunction2("abspath",
 	                            abspath,
 	                            "simfdb/backups/five/../three/../four",
 	                            true,
 	                            false,


Yep. you just a vector of tuples. and you write the test before and iterate over the fectory and call testPath2Function and collect errors. But it is out of scope for this pr i guess.

ghost · 2024-08-15T14:31:11Z

Given complexity of this PR and refactor, not sure if it's a better idea to split this apart. If so, then I'm not sure how building it would be possible since most of Platform.h is referenced elsewhere. I'm also new to project so would appreciate any best practice suggestions / If the current approach is encouraged.

xis19 · 2024-08-15T15:14:16Z

Given complexity of this PR and refactor, not sure if it's a better idea to split this apart. If so, then I'm not sure how building it would be possible since most of Platform.h is referenced elsewhere. I'm also new to project so would appreciate any best practice suggestions / If the current approach is encouraged.

Noting that your change are not triggering CI/CD (it is because you are not in the FDB team yet), I would suggest if you can do a clang-format on your code, fix the possible format issue, and let me trigger the CI pipeline to see what happens right now.

foundationdb-ci · 2024-08-15T16:15:36Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: b635a2a
Duration 0:04:39
Result: ❌ FAILED
Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-19T18:12:14Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: 6fda281
Duration 0:03:51
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-19T18:13:06Z

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: 6fda281
Duration 0:04:47
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-19T18:13:07Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 6fda281
Duration 0:04:44
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-19T18:13:08Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 6fda281
Duration 0:04:49
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2024-08-19T18:13:12Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 6fda281
Duration 0:04:50
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-19T18:14:02Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: 6fda281
Duration 0:05:43
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-19T18:15:01Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: 6fda281
Duration 0:06:37
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

xis19 · 2024-08-19T18:53:13Z

Okay, it seems there are some compile errors. One of them is that you need to let TraceEvent system support std::filesystem::path. Perhaps I could give a patch for this.

FYI:

THE FOLLOWING FILES NEED TO BE FORMATTED

flow/include/flow/Traceable.h

ghost · 2024-08-20T15:48:33Z

Okay, it seems there are some compile errors. One of them is that you need to let TraceEvent system support std::filesystem::path. Perhaps I could give a patch for this.

FYI:
THE FOLLOWING FILES NEED TO BE FORMATTED

flow/include/flow/Traceable.h

Strange.. I ran clang-format locally and it doesn't seem to be complaining about it. Will take a closer look, thank you for this info!

xis19 · 2024-08-20T22:36:35Z

flow/include/flow/Traceable.h

+		static std::string toString(type value) {                                                                      \
+			return format(fmt, value);                                                                                 \
+		}                                                                                                              \


I would suspect these lines caused the formatting problem. Can you please revert?

xis19 · 2024-08-20T22:46:06Z

flow/include/flow/Traceable.h

+	template <class Str>
+	static std::string toString(std::filesystem::path& value) {
+		return value.string();
+	}
+


I see you are trying to "overload" the toString method. However, contrary to intuition, C++ function templates do not overload. So there is no reason you add the template here. Herb Sutter has a very comprehensive explanation on this topic: http://www.gotw.ca/gotw/049.htm. Plus you are trying to capture value using a non-const reference. This causes all r-value calls fail, e.g. toString(std::filesystem::path()) will not compile. Usually a better choice is to use const std::filesystem::path& if std::filesystem::path::string() is const-constrainted. The other choice is to use rvalue-reference, which could be more flexible, yet more subtle and difficult to understand (sorry I do not know your level of knowledge of C++, if you understand rvalue-reference please ignore what i said.) In this particular situation, I would suggest you implement the template struct TraceableString<std::system::path> rather than this "overload". Another choice is to use

if constexpr (std::is_same<Str, std::filesystem::path>::value) { return value.string() } else { // old code }

but of course this is not recommended.

Thank you so much for providing the in-depth explanation... This is super helpful! Let me add this change here and revert suggestion above

Also, had a few concerns; I think that this issue may be beyond my capabilities. I'd like to still work on it but this would take a considerably a lot more time for me. At this point I'm considering to go back on previous commits to test out smaller changes instead of what is here since it's a bit too much to trace what's breaking what.

Curious to hear your thoughts on this :)

Do whatever you want as you are not paid to work on this project.

I help if it is needed but at the end of week, what are your concern? Avoid complexity.

I help if it is needed but at the end of week, what are your concern? Avoid complexity.

Remove this function and implement template specialization TraceableString<std::system::path>

foundationdb-ci · 2024-08-22T08:16:41Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: f7c1a55
Duration 0:03:37
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-22T08:17:36Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: f7c1a55
Duration 0:04:34
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-22T08:17:37Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: f7c1a55
Duration 0:04:35
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-22T08:17:45Z

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: f7c1a55
Duration 0:04:40
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-22T08:17:49Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: f7c1a55
Duration 0:04:46
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2024-08-22T08:19:24Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: f7c1a55
Duration 0:06:22
Result: ❌ FAILED
Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-08-22T08:19:27Z