refactor: Replace fs::unique_path with GetUniquePath(path) calls by kiminuo · Pull Request #21052 · bitcoin/bitcoin

kiminuo · 2021-02-01T12:47:01Z

This PR makes it easier in #20744 to remove our dependency on the boost::filesystem::unique_path() function which does not have a direct equivalent in C++17.

This PR attempts to re-implement boost::filesystem::unique_path() as GetUniquePath(path) but the implementations are not meant to be the same.

Note:

Boost 1.75.0 implementation of unique_path: https://github.com/boostorg/filesystem/blob/9cab675b71e98706886a87afe7c19eb9da568961/src/unique_path.cpp#L235
In the previous implementation, I attempted to add:
```
fs::path GetUniquePath(const fs::path& base)
{
    FastRandomContext rnd;
    fs::path tmpFile = base / HexStr(rnd.randbytes(8));
    return tmpFile;
}
```
to fs.cpp but this leads to a circular dependency: "fs -> random -> logging -> fs". That is why the modified implementation adds a new file.

DrahtBot · 2021-02-01T13:27:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Use std::filesystem. Remove Boost Filesystem & System #20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj · 2021-02-02T12:04:31Z

Concept ACK

However, this introduces a circular dependency right now:

A new circular dependency in the form of "fs -> random -> logging -> fs" appears to have been introduced.

kiminuo · 2021-02-03T10:17:20Z

src/test/fs_tests.cpp

+        BOOST_CHECK_EQUAL(tmpfolder, p3.parent_path());
+
+        // Ensure that generated paths are actually different.
+        BOOST_CHECK(p1 != p2);


I have left this intentionally very simple. One can certainly come up with many different ways to make this more robust but I would like to solicit for feedback for this.

kiminuo · 2021-02-03T10:17:50Z

However, this introduces a circular dependency right now:

I have changed the implementation. So this is fixed.

…tem::unique_path() which is not available in std::filesystem.

practicalswift · 2021-02-05T08:54:25Z

Concept ACK

Thanks for paving the way for further Boost removal!

ryanofsky

Code review ACK 1bca2aa. It's a simple change and extra test coverage is nice

ryanofsky · 2021-02-05T16:17:21Z

src/test/fs_tests.cpp

 }

-BOOST_AUTO_TEST_SUITE_END()
+BOOST_AUTO_TEST_SUITE_END()


In commit "Introduce GetUniquePath(base) helper method to replace boost::filesystem::unique_path() which is not available in std::filesystem." (1bca2aa)

Not a problem but there is an extraneous whitespace change here

laanwj · 2021-02-09T21:19:15Z

Code review ACK 1bca2aa

…Path(path) calls 1bca2aa Introduce GetUniquePath(base) helper method to replace boost::filesystem::unique_path() which is not available in std::filesystem. (Kiminuo) Pull request description: This PR makes it easier in bitcoin#20744 to remove our dependency on the `boost::filesystem::unique_path()` function which does not have a direct equivalent in C++17. This PR attempts to re-implement `boost::filesystem::unique_path()` as `GetUniquePath(path)` but the implementations are not meant to be the same. Note: * Boost 1.75.0 implementation of `unique_path`: https://github.com/boostorg/filesystem/blob/9cab675b71e98706886a87afe7c19eb9da568961/src/unique_path.cpp#L235 * In the previous implementation, I attempted to add: ```cpp fs::path GetUniquePath(const fs::path& base) { FastRandomContext rnd; fs::path tmpFile = base / HexStr(rnd.randbytes(8)); return tmpFile; } ``` to `fs.cpp` but this leads to a circular dependency: "fs -> random -> logging -> fs". That is why the modified implementation adds a new file. ACKs for top commit: laanwj: Code review ACK 1bca2aa ryanofsky: Code review ACK 1bca2aa. It's a simple change and extra test coverage is nice Tree-SHA512: f324bdf0e254160c616b5033c3ece33d87db23eb0135acee99346ade7b5cf0d30f3ceefe359a25a8e9b53ba8e4419f459c2bdd369e10fc0152ce95031d1f221c

… calls

merge bitcoin#16117, bitcoin#18358, bitcoin#17383, bitcoin#21052, bitcoin#14424, bitcoin#15159, bitcoin#14689, bitcoin#14978, partial bitcoin#16908, bitcoin#14978, bitcoin#13932: Auxillary Backports

This was missed in bitcoin#21052.

69a439b doc: add missing copyright header to getuniquepath.cpp (fanquake) Pull request description: This was missed in #21052. ACKs for top commit: hebasto: ACK 69a439b, but a scripted-diff using `copyright_header.py insert` looks preferable. Tree-SHA512: 1a75ffd93fa8e5e83821e1fe984353422740ebf9e203dc873debf4dffec0e23f55a1e31f806dd2d66087d3620f6dc447af69f9124f0bcc6676ef91ee35374832

…th.cpp 69a439b doc: add missing copyright header to getuniquepath.cpp (fanquake) Pull request description: This was missed in bitcoin#21052. ACKs for top commit: hebasto: ACK 69a439b, but a scripted-diff using `copyright_header.py insert` looks preferable. Tree-SHA512: 1a75ffd93fa8e5e83821e1fe984353422740ebf9e203dc873debf4dffec0e23f55a1e31f806dd2d66087d3620f6dc447af69f9124f0bcc6676ef91ee35374832

… calls

…th.cpp 69a439b doc: add missing copyright header to getuniquepath.cpp (fanquake) Pull request description: This was missed in bitcoin#21052. ACKs for top commit: hebasto: ACK 69a439b, but a scripted-diff using `copyright_header.py insert` looks preferable. Tree-SHA512: 1a75ffd93fa8e5e83821e1fe984353422740ebf9e203dc873debf4dffec0e23f55a1e31f806dd2d66087d3620f6dc447af69f9124f0bcc6676ef91ee35374832

DrahtBot added the Utils/log/libs label Feb 1, 2021

This was referenced Feb 1, 2021

Use std::filesystem. Remove Boost Filesystem & System #20744

Merged

util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715

Merged

fanquake requested a review from ryanofsky February 2, 2021 05:48

fanquake changed the title ~~[draft] refactor: Replace fs::unique_path with GetUniquePath(path) calls~~ refactor: Replace fs::unique_path with GetUniquePath(path) calls Feb 2, 2021

kiminuo force-pushed the feature/fs-unique-path branch from f83bb3a to 805f888 Compare February 2, 2021 23:02

kiminuo commented Feb 3, 2021

View reviewed changes

kiminuo marked this pull request as ready for review February 3, 2021 10:18

DrahtBot added the Needs rebase label Feb 4, 2021

Introduce GetUniquePath(base) helper method to replace boost::filesys…

1bca2aa

…tem::unique_path() which is not available in std::filesystem.

kiminuo force-pushed the feature/fs-unique-path branch from 0e1b085 to 1bca2aa Compare February 4, 2021 10:38

DrahtBot removed the Needs rebase label Feb 4, 2021

ryanofsky approved these changes Feb 5, 2021

View reviewed changes

laanwj merged commit d202054 into bitcoin:master Feb 9, 2021

kiminuo deleted the feature/fs-unique-path branch February 9, 2021 21:24

maflcko mentioned this pull request Feb 10, 2021

doc: Add missing copyright header to util/getuniquepath #21139

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Jul 16, 2021

merge bitcoin#21052: Replace fs::unique_path with GetUniquePath(path)…

e63a2cb

… calls

kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021

merge bitcoin#21052: Replace fs::unique_path with GetUniquePath(path)…

5e0cc08

… calls

kwvg added a commit to kwvg/dash that referenced this pull request Aug 9, 2021

merge bitcoin#21052: Replace fs::unique_path with GetUniquePath(path)…

09ace18

… calls

fanquake added a commit to fanquake/bitcoin that referenced this pull request Sep 8, 2021

doc: add missing copyright header to getuniquepath.cpp

69a439b

This was missed in bitcoin#21052.

fanquake mentioned this pull request Sep 8, 2021

doc: add missing copyright header to getuniquepath.cpp #22916

Merged

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 1, 2022

merge bitcoin#21052: Replace fs::unique_path with GetUniquePath(path)…

2f7541e

… calls

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Replace fs::unique_path with GetUniquePath(path) calls#21052

refactor: Replace fs::unique_path with GetUniquePath(path) calls#21052
laanwj merged 1 commit intobitcoin:masterfrom
kiminuo:feature/fs-unique-path

kiminuo commented Feb 1, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Feb 1, 2021 •

edited

Loading

Uh oh!

laanwj commented Feb 2, 2021

Uh oh!

kiminuo Feb 3, 2021

Uh oh!

kiminuo commented Feb 3, 2021

Uh oh!

practicalswift commented Feb 5, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Feb 5, 2021

Uh oh!

laanwj commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

kiminuo commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Feb 2, 2021

Uh oh!

kiminuo Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Feb 3, 2021

Uh oh!

practicalswift commented Feb 5, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kiminuo commented Feb 1, 2021 •

edited

Loading

DrahtBot commented Feb 1, 2021 •

edited

Loading