util: use stronger-guarantee rename method by vasild · Pull Request #20435 · bitcoin/bitcoin

vasild · 2020-11-20T14:37:54Z

Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.

maflcko · 2020-11-20T14:43:42Z

Please update doc/dependencies to gcc 8 (from 7), as std::fs is not included in gcc 7

Edit: And clang-7

vasild · 2020-11-20T15:27:45Z

Updated. I am not sure if this PR shouldn't be put on hold until CentOS 23 starts shipping with a recent enough compiler?

maflcko · 2020-11-20T15:29:32Z

Maybe adding -lstdc++fs to the build flags via configure by default fixes the compile errors?

laanwj · 2020-11-23T10:10:26Z

Concept ACK! That's another WIN32 specific hack less.

But yes it's probably going to be a looong time before this can be merged.

vasild · 2020-11-23T10:34:29Z

I will leave this as is (open, with failing CI) until the compilers we support catch up with C++17 filesystem support.

DrahtBot · 2020-11-23T11:11:03Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

vasild · 2020-11-24T13:50:39Z

Removed the changes to doc/dependencies.md as those will be carried by #20460 (bumping minimum compiler version requirements), also they caused conflicts.

luke-jr · 2020-11-25T00:17:03Z

:>Removed the changes to doc/dependencies.md as those will be carried by #20460 (bumping minimum compiler version requirements), also they caused conflicts.

You should probably rebase this PR on top of that then...

vasild · 2020-11-25T09:03:11Z

@luke-jr that's right, but #20460 is just an issue/idea, without a patch (yet)...

laanwj · 2020-12-03T12:30:22Z

I will leave this as is (open, with failing CI) until the compilers we support catch up with C++17 filesystem support.

I don't think labeling this as "waiting for author" is quite right.

maflcko · 2020-12-03T12:46:30Z

This pull is waiting for the author to rebase on the pull that fixes #20460 . Is there a better label?

vasild · 2020-12-03T14:38:25Z

There is no fix for the issue #20460 yet.

This PR is waiting for newer compilers to hit the master branch (docs and ci).

…irements 182de7b ci: update minimum compiler requirements for std::filesystem (fanquake) 04f5baf doc: update minimum compiler requirements for std::filesystem (fanquake) Pull request description: This increases the minimum required compiler versions to Clang 7 and GCC 8.1. This has been split out of #20744 (migration to `std::filesystem`), as it's also a requirement for some other changes, such as #20452 or #20457 which want to make use of `std::from_chars`. As well as #20435, which is also `std::filesystem` related. Given that the `std::filesystem` changes are moving ahead, splitting out this change to let other PRs take advantage of the new requirements seems worthwhile. Clang 7 has been available in Debian since [Stretch (oldoldstable)](https://packages.debian.org/stretch/clang-7) and in Ubuntu since [Bionic (18.04)](https://packages.ubuntu.com/bionic-updates/clang-7). GCC 8 has been available in Debian since [Buster (oldstable)](https://packages.debian.org/buster/gcc) and in Ubuntu since [Bionic (18.04)](https://packages.ubuntu.com/bionic/gcc-8). CentOS 8 also packages GCC 8. The CI changes here give us one build with GCC 8, and another using Clang 7 on top of libc++. Note that the minimum required libc++ in dependencies.md is unchanged as, at least for `<filesystem>`, and the `*_chars` use cases, libc++ 7 [should be sufficient](https://en.cppreference.com/w/cpp/compiler_support/17). I've tested that building `<filesystem>` code using Clang 7 & libc++ works. i.e `clang++-7 -std=c++17 fs.cpp -stdlib=libc++ -lc++fs`. Also that building `<filesystem>` code with Clang 7 and libstdc++ 8 works. i.e `clang++-7 -std=c++17 fs.cpp -lstdc++fs`. ACKs for top commit: MarcoFalke: review ACK 182de7b Tree-SHA512: 5bc151c4be58005711eed6bd8a091f3417f75a0218c11c08cffff9d749edadd965726bb7856a8e693e96e69ed0596989cda1aac4b29fb6d30705b1687a5b3363

fanquake · 2021-09-28T06:16:45Z

There is no fix for the issue #20460 yet.

If you want, you can now rebase on top of #20744.

src/util/system.cpp

maflcko · 2021-09-28T06:26:21Z

If you want, you can now rebase on top of #20744.

I think a rebase is not (strictly) needed, just a (force) push to re-trigger CI.

fanquake · 2021-09-28T08:03:36Z

I think a rebase is not (strictly) needed, just a (force) push to re-trigger CI.

Sure, but this can't be merged before #20744 in any case.

vasild · 2021-09-28T08:05:37Z

166b1542d7...99b151df9c: rebase

This PR is waiting for newer compilers to hit the master branch

#23060 did just that (merged).

I don't think this PR depends on #20744. @fanquake, why do you think this PR cannot be merged before #20744?

fanquake · 2021-09-28T08:10:00Z

I don't think this PR depends on #20744. @fanquake, why do you think this PR cannot be merged before #20744?

Because we are either using boost::filesystem or std::filesystem. We're not going to have the repo exist in some awkward middle ground where we are using both. In any case, using std::filesystem is not necessarily straight forward, and likely requires the changes in #22937 before it can be used.

fanquake · 2021-09-28T08:13:06Z

Another reason is that this doesn't have any build system support for properly using std::filesystem, and the additional linker flags that may be needed, i.e 7219288.

ryanofsky

Conditional code review ACK 99b151df9c074f60150c5eb777ed98bfd7118e91 if this is rebased and .string() is replaced by .native() (see comment below).

This is just a review of the c++ code, disregarding build issues. IIUC the new rename function may not be callable on all platforms without some of the build changes in #20744.

src/util/system.cpp

Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case.

vasild · 2021-11-02T13:21:35Z

99b151df9c...a9ab42ca97: rebase due to conflicts and pick up a suggestion.

This now depends on #20744 in two ways:

Build system changes to make it compile on all platforms
Assumes that our custom fs::path is std::filesystem::path (passes a variable of type fs::path to std::filesystem::rename()).

Invalidates ACK from @ryanofsky (conditional)

ryanofsky

Code review ACK a9ab42c assuming #20744 is merged first (it requires #20744 to build)

DrahtBot · 2022-02-03T16:12:32Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

fanquake · 2022-02-10T08:18:35Z

I've rebased this in #24308.

fanquake · 2022-02-10T09:19:46Z

Going to close this in favour of #24308.

ee822d8 util: use stronger-guarantee rename method (Vasil Dimov) Pull request description: Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case. This is a rebase of #20435 by vasild. ACKs for top commit: MarcoFalke: review ACK ee822d8 hebasto: Approach ACK ee822d8. vasild: ACK ee822d8 Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a

ee822d8 util: use stronger-guarantee rename method (Vasil Dimov) Pull request description: Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case. This is a rebase of bitcoin#20435 by vasild. ACKs for top commit: MarcoFalke: review ACK ee822d8 hebasto: Approach ACK ee822d8. vasild: ACK ee822d8 Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a

vasild force-pushed the rename branch from 375a8b6 to 6e37c03 Compare November 20, 2020 15:25

DrahtBot added Docs Utils/log/libs labels Nov 20, 2020

maflcko removed the Docs label Nov 20, 2020

maflcko added the Waiting for author label Nov 23, 2020

maflcko mentioned this pull request Nov 23, 2020

C++17 std::fs #20460

Closed

DrahtBot mentioned this pull request Nov 23, 2020

build: Qt 5.15.x #19716

Closed

DrahtBot added the Needs rebase label Nov 24, 2020

vasild force-pushed the rename branch from 6e37c03 to 166b154 Compare November 24, 2020 13:49

DrahtBot removed the Needs rebase label Nov 24, 2020

laanwj added this to the Future milestone Dec 3, 2020

maflcko added Waiting for other and removed Waiting for author labels Dec 15, 2020

DrahtBot mentioned this pull request Mar 23, 2021

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

DrahtBot mentioned this pull request Sep 10, 2021

refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method #22937

Merged

fanquake mentioned this pull request Sep 22, 2021

release: increase minimum compiler and lib(std)c++ requirements #23060

Merged

maflcko removed the Waiting for other label Sep 28, 2021

maflcko reviewed Sep 28, 2021

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

vasild force-pushed the rename branch from 166b154 to 99b151d Compare September 28, 2021 08:02

DrahtBot added the Needs rebase label Oct 15, 2021

ryanofsky approved these changes Nov 1, 2021

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

util: use stronger-guarantee rename method

a9ab42c

Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case.

vasild force-pushed the rename branch from 99b151d to a9ab42c Compare November 2, 2021 11:27

DrahtBot removed the Needs rebase label Nov 2, 2021

ryanofsky approved these changes Nov 2, 2021

View reviewed changes

DrahtBot added the Needs rebase label Feb 3, 2022

fanquake mentioned this pull request Feb 10, 2022

util: use stronger-guarantee rename method #24308

Merged

fanquake closed this Feb 10, 2022

vasild deleted the rename branch February 10, 2022 13:24

bitcoin locked and limited conversation to collaborators Feb 10, 2023

maflcko removed this from the Future milestone Jul 23, 2025

Conversation

vasild commented Nov 20, 2020

Uh oh!

maflcko commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Nov 20, 2020

Uh oh!

maflcko commented Nov 20, 2020

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

vasild commented Nov 23, 2020

Uh oh!

DrahtBot commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild commented Nov 24, 2020

Uh oh!

luke-jr commented Nov 25, 2020

Uh oh!

vasild commented Nov 25, 2020

Uh oh!

laanwj commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 3, 2020

Uh oh!

vasild commented Dec 3, 2020

Uh oh!

fanquake commented Sep 28, 2021

Uh oh!

Uh oh!

maflcko commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Sep 28, 2021

Uh oh!

vasild commented Sep 28, 2021

Uh oh!

fanquake commented Sep 28, 2021

Uh oh!

fanquake commented Sep 28, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 3, 2022

Uh oh!

fanquake commented Feb 10, 2022

Uh oh!

fanquake commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maflcko commented Nov 20, 2020 •

edited

Loading

DrahtBot commented Nov 23, 2020 •

edited

Loading

laanwj commented Dec 3, 2020 •

edited

Loading

maflcko commented Sep 28, 2021 •

edited

Loading

vasild commented Nov 2, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading