util: use stronger-guarantee rename method #20435

pull vasild wants to merge 1 commits into bitcoin:master from vasild:rename changing 2 files +11 −7
  1. vasild commented at 2:37 pm on November 20, 2020: member
    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.
  2. MarcoFalke commented at 2:43 pm on November 20, 2020: member

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

    Edit: And clang-7

  3. vasild force-pushed on Nov 20, 2020
  4. vasild commented at 3:27 pm on November 20, 2020: member
    Updated. I am not sure if this PR shouldn’t be put on hold until CentOS 23 starts shipping with a recent enough compiler?
  5. MarcoFalke commented at 3:29 pm on November 20, 2020: member
    Maybe adding -lstdc++fs to the build flags via configure by default fixes the compile errors?
  6. DrahtBot added the label Docs on Nov 20, 2020
  7. DrahtBot added the label Utils/log/libs on Nov 20, 2020
  8. MarcoFalke removed the label Docs on Nov 20, 2020
  9. laanwj commented at 10:10 am on November 23, 2020: member

    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.

  10. vasild commented at 10:34 am on November 23, 2020: member
    I will leave this as is (open, with failing CI) until the compilers we support catch up with C++17 filesystem support.
  11. MarcoFalke added the label Waiting for author on Nov 23, 2020
  12. DrahtBot commented at 11:11 am on November 23, 2020: member

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

    Conflicts

    No conflicts as of last run.

  13. DrahtBot added the label Needs rebase on Nov 24, 2020
  14. vasild force-pushed on Nov 24, 2020
  15. vasild commented at 1:50 pm on November 24, 2020: member
    Removed the changes to doc/dependencies.md as those will be carried by #20460 (bumping minimum compiler version requirements), also they caused conflicts.
  16. DrahtBot removed the label Needs rebase on Nov 24, 2020
  17. luke-jr commented at 0:17 am on November 25, 2020: member

    :>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…

  18. vasild commented at 9:03 am on November 25, 2020: member
    @luke-jr that’s right, but #20460 is just an issue/idea, without a patch (yet)…
  19. laanwj commented at 12:30 pm on December 3, 2020: member

    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.

  20. laanwj added this to the milestone Future on Dec 3, 2020
  21. MarcoFalke commented at 12:46 pm on December 3, 2020: member
    This pull is waiting for the author to rebase on the pull that fixes #20460 . Is there a better label?
  22. vasild commented at 2:38 pm on December 3, 2020: member

    There is no fix for the issue #20460 yet.

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

  23. MarcoFalke removed the label Waiting for author on Dec 15, 2020
  24. MarcoFalke added the label Waiting for other on Dec 15, 2020
  25. fanquake referenced this in commit 67eae69f3f on Sep 28, 2021
  26. fanquake commented at 6:16 am on September 28, 2021: member

    There is no fix for the issue #20460 yet.

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

  27. MarcoFalke removed the label Waiting for other on Sep 28, 2021
  28. in src/util/system.cpp:1013 in 166b1542d7 outdated 
    1006@@ -1005,15 +1007,15 @@ void ArgsManager::LogArgs() const
1007     logArgsPrefix("Command-line arg:", "", m_settings.command_line_options);
1008 }
1009 
1010+/**
1011+ * Rename src to dest.
1012+ * @return true if the rename was successful.
1013+ */
    MarcoFalke commented at 6:21 am on September 28, 2021:
    Shouldn’t doxygen be in the .h file?
    vasild commented at 8:06 am on September 28, 2021:
    Right, moved.
  29. MarcoFalke commented at 6:26 am on September 28, 2021: member

    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.

  30. vasild force-pushed on Sep 28, 2021
  31. fanquake commented at 8:03 am on September 28, 2021: member

    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.

  32. vasild commented at 8:05 am on September 28, 2021: member

    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?

  33. fanquake commented at 8:10 am on September 28, 2021: member

    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.

  34. fanquake commented at 8:13 am on September 28, 2021: member
    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 https://github.com/bitcoin/bitcoin/pull/20744/commits/721928821f1623265db9c07f2fcef49a356d477f.
  35. DrahtBot added the label Needs rebase on Oct 15, 2021
  36. in src/util/system.cpp:1069 in 99b151df9c outdated 
    1071-#else
1072-    int rc = std::rename(src.string().c_str(), dest.string().c_str());
1073-    return (rc == 0);
1074-#endif /* WIN32 */
1075+    std::error_code error;
1076+    std::filesystem::rename(src.string(), dest.string(), error);
    ryanofsky commented at 4:08 pm on November 1, 2021:

    In commit “util: use stronger-guarantee rename method” (99b151df9c074f60150c5eb777ed98bfd7118e91)

    This will result in narrow string conversion on windows, and after #22937 will be a compiler error. Should be easy to fix by changing to:

    0std::filesystem::rename(src.native(), dest.native(), error);

    This should also work after #20744, but after #20744 it could be simplified more to

    0std::filesystem::rename(src, dest, error);
    vasild commented at 1:18 pm on November 2, 2021:
    Used just src and dest, thanks!
  37. ryanofsky approved
  38. ryanofsky commented at 4:18 pm on November 1, 2021: member

    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.

  39. util: use stronger-guarantee rename method 
    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.
    a9ab42ca97
  40. vasild force-pushed on Nov 2, 2021
  41. DrahtBot removed the label Needs rebase on Nov 2, 2021
  42. vasild commented at 1:21 pm on November 2, 2021: member

    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)

  43. ryanofsky approved
  44. ryanofsky commented at 3:39 pm on November 2, 2021: member
    Code review ACK a9ab42ca973e1841ac125b0f64eaa922df240c08 assuming #20744 is merged first (it requires #20744 to build)
  45. DrahtBot added the label Needs rebase on Feb 3, 2022
  46. DrahtBot commented at 4:12 pm on February 3, 2022: member

    🐙 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”.

  47. fanquake commented at 8:18 am on February 10, 2022: member
    I’ve rebased this in #24308.
  48. fanquake commented at 9:19 am on February 10, 2022: member
    Going to close this in favour of #24308.
  49. fanquake closed this on Feb 10, 2022
  50. vasild deleted the branch on Feb 10, 2022
  51. MarcoFalke referenced this in commit b79c40b057 on Feb 11, 2022
  52. sidhujag referenced this in commit ed278f1df3 on Feb 11, 2022
  53. DrahtBot locked this on Feb 10, 2023


vasild MarcoFalke laanwj DrahtBot luke-jr fanquake ryanofsky

Labels
Utils/log/libs Needs rebase

Milestone
Future

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me