foundationdb icon indicating copy to clipboard operation
foundationdb copied to clipboard
verified publisher icon apple

WIP: Fix warnings for long long or int64_t format specifiers by switching to fmt::print*

Open sepeth opened this issue 1 year ago • 22 comments

Probably not a big issue and not sure if anyone is running FDB on 32 bit, but this still helps to suppress perhaps unnecessary warnings.

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)

sepeth avatar Aug 14 '24 09:08 sepeth

This change is great. I would suggest we move to std::cout so we do not need to care about the types of the variables as long as operator<< is implemented properly.

xis19 avatar Aug 14 '24 16:08 xis19

I tried that now but I think it looks worse in most cases:

Screenshot 2024-08-15 at 20 08 02

Is std::print an option? Or at least std::format which is in C++20?

sepeth avatar Aug 15 '24 17:08 sepeth

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

  • Commit ID: 618f1ce351f1670e088dbc1227f3057c49d8e52c
  • Duration 0:22:21
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Aug 16 '24 21:08 foundationdb-ci

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 618f1ce351f1670e088dbc1227f3057c49d8e52c
  • Duration 0:51:35
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Aug 16 '24 21:08 foundationdb-ci

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

  • Commit ID: 618f1ce351f1670e088dbc1227f3057c49d8e52c
  • Duration 0:52:05
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Aug 16 '24 21:08 foundationdb-ci

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

  • Commit ID: 618f1ce351f1670e088dbc1227f3057c49d8e52c
  • Duration 0:59:15
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • 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 avatar Aug 16 '24 21:08 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 618f1ce351f1670e088dbc1227f3057c49d8e52c
  • Duration 1:03:00
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Aug 16 '24 21:08 foundationdb-ci

I tried that now but I think it looks worse in most cases:

Screenshot 2024-08-15 at 20 08 02 Is std::print an option? Or at least std::format which is in C++20?

Unfortunately at this stage we are using gcc-11, so there is no std::format available. I should be able to upgrade this to some reasonable version which supports more fatures in C++20, e.g., 13, but I am stuck on somewhere else, so this may not happen very soon. I personally think fmt would be a good choice at this stage.

xis19 avatar Aug 19 '24 18:08 xis19

I should be able to upgrade this to some reasonable version which supports more fatures in C++20, e.g., 13, but I am stuck on somewhere else, so this may not happen very soon.

Happy to help if you don't mind me taking it. Does this mainly involve updating Rocky Linux image in fdb-build-support?

sepeth avatar Aug 19 '24 19:08 sepeth

I should be able to upgrade this to some reasonable version which supports more fatures in C++20, e.g., 13, but I am stuck on somewhere else, so this may not happen very soon.

Happy to help if you don't mind me taking it. Does this mainly involve updating Rocky Linux image in fdb-build-support?

Partly, but it is involved in some Apple internal issue so I guess I have to be the person doing this job, due to certain policies, and this upgrade may not happen soon.

xis19 avatar Aug 19 '24 20:08 xis19

Is std::print an option? Or at least std::format which is in C++20?

Unfortunately at this stage we are using gcc-11, so there is no std::format available. I should be able to upgrade this to some reasonable version which supports more fatures in C++20, e.g., 13, but I am stuck on somewhere else, so this may not happen very soon. I personally think fmt would be a good choice at this stage.

I agree that we can use fmt::format for now. It's used extensively in the codebase. Does that work @sepeth? If we can do that, then if at some point we migrate to std::format, your code changes can be part of that migration (likely an automated script that will do it so shouldn't be extra overhead).

spraza avatar Aug 19 '24 20:08 spraza

Fantastic, I didn't notice fmt ^-^

May you also please have a look at #11566? This one is about warnings, while without that one linking fails on macOS.

sepeth avatar Aug 20 '24 11:08 sepeth

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

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:06:05
  • Result: :x: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /opt/homebrew/bin/bash --login -c ./build_pr_macos.sh. 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 avatar Aug 20 '24 22:08 foundationdb-ci

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

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:07:14
  • Result: :x: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all. 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 avatar Aug 20 '24 23:08 foundationdb-ci

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

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:10:39
  • Result: :x: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. 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 avatar Aug 20 '24 23:08 foundationdb-ci

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:28:24
  • Result: :x: 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 avatar Aug 20 '24 23:08 foundationdb-ci

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

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:36:10
  • Result: :x: 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)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci avatar Aug 20 '24 23:08 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:36:29
  • Result: :x: 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 avatar Aug 20 '24 23:08 foundationdb-ci

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

  • Commit ID: ee04dd4ec3eb719e01efafa6105f7047ba47b8bc
  • Duration 0:39:46
  • Result: :x: 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 avatar Aug 20 '24 23:08 foundationdb-ci

CI is failing, one example failure in clang build is:

/codebuild/output/src1940136774/src/github.com/apple/foundationdb/fdbcli/GetAuditStatusCommand.actor.cpp:73:8: error: no member named 'println' in namespace '
fmt'
                fmt::println("Finished range count: {}", finishCount);
                ~~~~~^

I don't see println API:

# returns empty output
$ grep -r ".*println.*" contrib/fmt-8.1.1/

I do see it in the latest fmt code: https://github.com/fmtlib/fmt/blob/master/include/fmt/base.h#L3155.

spraza avatar Aug 23 '24 18:08 spraza

CI is failing, one example failure in clang build is:

/codebuild/output/src1940136774/src/github.com/apple/foundationdb/fdbcli/GetAuditStatusCommand.actor.cpp:73:8: error: no member named 'println' in namespace '
fmt'
                fmt::println("Finished range count: {}", finishCount);
                ~~~~~^

I don't see println API:

# returns empty output
$ grep -r ".*println.*" contrib/fmt-8.1.1/

I do see it in the latest fmt code: https://github.com/fmtlib/fmt/blob/master/include/fmt/base.h#L3155.

You are right, I only noticed after PR checks, that's why I changed the title to WIP. fmt::println is only added in 10.0.0, but FDB has 8.1.1 as you mentioned.

Also, I actually upgraded fmt locally to 11.0.2, hence my mistake about fmt::println, but I didn't add here yet due to CycleTest in tests/default.txt started to fail in Simulation -- Rate below desired rate. When I checked Cycle.Metrics in the output, I realized the transactions metrics dropped. I couldn't make sense of it yet. Apparently there was a performance regression with 11, but it says it got fixed with 11.0.2 and it seems unrelated (see 11.0.2 Release Notes).

Also, right now, fmt source is in contrib, my change was to get it via cmake's FetchContent. First, it might be suspicious for a new contributor to change that much code when it is in contrib/, and it would be too much burden on the reviewers, and it would be easier for anybody to upgrade it later (via just bumping the version in a cmake file). Please don't hesitate to let me know if this is something that would be welcome or unwelcome.

sepeth avatar Aug 23 '24 18:08 sepeth

Created #11601 for fmt upgrade in case it is useful.

sepeth avatar Aug 23 '24 19:08 sepeth

@sepeth Great that https://github.com/apple/foundationdb/pull/11601 was merged so now we can use fmt::println. I am trying to trigger the CI. Once it passes, will accept this PR.

spraza avatar Sep 12 '24 17:09 spraza

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

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 0:07:22
  • Result: :x: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /opt/homebrew/bin/bash --login -c ./build_pr_macos.sh. 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 avatar Sep 12 '24 17:09 foundationdb-ci

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

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 0:12:02
  • Result: :x: FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. 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 avatar Sep 12 '24 18:09 foundationdb-ci

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

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 0:21:59
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Sep 12 '24 18:09 foundationdb-ci

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

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 0:52:24
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Sep 12 '24 18:09 foundationdb-ci

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

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 0:58:04
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • 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 avatar Sep 12 '24 18:09 foundationdb-ci

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 1:10:23
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Sep 12 '24 19:09 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: ece5ac4df4c6a6bca75a5cceb35d64736e315e9a
  • Duration 1:14:31
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci avatar Sep 12 '24 19:09 foundationdb-ci