util: Revert back `MoveFileExW` call for MinGW-w64

hebasto commented at 3:47 pm on February 13, 2022: member

Unfortunately, bitcoin/bitcoin#24308 introduced a regression for mingw builds.

The root of the problem is a broken implementation of std::filesystem::rename. In particular, the expected behavior

If old_p is a non-directory file, then new_p must be … existing non-directory file: new_p is first deleted…

fails with the “File exists” error.

This PR reverts back the MoveFileExW call, and adds the suggested unit test.

hebasto force-pushed on Feb 13, 2022

fanquake commented at 4:00 pm on February 13, 2022: member

The root of the problem is a broken implementation of std::filesystem::rename.

What version is broken? Is there an upstream issue / bug report?

DrahtBot added the label Utils/log/libs on Feb 13, 2022

MarcoFalke commented at 7:17 pm on February 13, 2022: member

Does the unit test fail on current master?

I am surprised to see that native windows and windows cross builds in the CI pass on current master with the presumed bug included.

MarcoFalke commented at 7:18 pm on February 13, 2022: member

Ah, it fails: https://cirrus-ci.com/task/6670169794674688?logs=ci#L4536

Concept ACK

laanwj commented at 8:15 am on February 14, 2022: member

Good catch… Leave it to windows libraries to not actually implement the standard as it is defined.

MarcoFalke commented at 8:20 am on February 14, 2022: member

At one point it might be better to drop support for mingw and use clang instead for cross compiling. With all the bugs we are seeing and other projects moving to clang I am not convinced this is a good long term choice anymore.

in src/util/system.cpp:1072 in 15a043dcee outdated

1061@@ -1062,9 +1062,14 @@ void ArgsManager::LogArgs() const
1062 
1063 bool RenameOver(fs::path src, fs::path dest)
1064 {
1065+#ifdef __MINGW64__
1066+    return MoveFileExW(src.wstring().c_str(), dest.wstring().c_str(),

laanwj commented at 9:08 am on February 14, 2022:

Might want to add a comment here specifying why this workaround exists, and we should probably file an upstream issue and refer to it, so it has a limited lifetime.

hebasto commented at 2:58 pm on February 14, 2022:

Thanks! Updated.

laanwj commented at 9:09 am on February 14, 2022: member

I didn’t realize the bug was Mingw only. So MSVC (or even clang) implements this correctly?

At one point it might be better to drop support for mingw and use clang instead for cross compiling.

Agree. If it’s less buggy than mingw-w64, I think it would be good to consider using clang for the windows build in a future release.

in src/test/fs_tests.cpp:151 in 15a043dcee outdated

148+    {
149+        std::ifstream file{path2};
150+        std::string contents;
151+        file >> contents;
152+        BOOST_CHECK_EQUAL(contents, path1_contents);
153+    }

vasild commented at 9:32 am on February 14, 2022:

It is a good practice to clean up after the test, even if it is a temporary folder. I forgot that, but woke up during the night with the thought “file cleanup was forgotten!”.

0    }
1    fs::remove(path2);

hebasto commented at 9:48 am on February 14, 2022:

Thanks! Updated.

vasild approved

vasild commented at 9:36 am on February 14, 2022: member

ACK 15a043dcee116f5273a262ad83853c791c50b9fa

vasild commented at 9:37 am on February 14, 2022: member

mingw32 works ok? Or we don’t support that?

hebasto commented at 9:41 am on February 14, 2022: member

mingw32 works ok? Or we don’t support that?

The latter, I guess.

hebasto force-pushed on Feb 14, 2022

hebasto commented at 9:47 am on February 14, 2022: member

Updated 15a043dcee116f5273a262ad83853c791c50b9fa -> 4d92107041c508cf7bca47dce088979e519fb8bc (pr24331.01 -> pr24331.02, diff):

addressed @vasild’s comment

vasild approved

vasild commented at 9:50 am on February 14, 2022: member

ACK 4d92107041c508cf7bca47dce088979e519fb8bc

fanquake commented at 9:51 am on February 14, 2022: member

What version is broken? Is there an upstream issue / bug report?

laanwj commented at 11:17 am on February 14, 2022: member

mingw32 works ok? Or we don’t support that?

afaik mingw-w64 was a fork years ago because mingw32 wasn’t really maintained anymore, i doubt they support newer C++ standards required

Edit: wait, i’m confused here (the confusing naming of the projects is the gift that keeps giving), it’s very possible that mingw-w64 defines __MINGW32__ in 32-bit mode? In any case we don’t support building for 32-bit windows at all so it doesn’t matter.

hebasto commented at 1:45 pm on February 14, 2022: member

it’s very possible that mingw-w64 defines __MINGW32__ in 32-bit mode?

AFAIK, __MINGW32__ is always defined, __MINGW64__ is defined in 64-bit mode only.

In any case we don’t support building for 32-bit windows at all so it doesn’t matter.

Yeap :)

hebasto force-pushed on Feb 14, 2022

util: Revert back MoveFileExW call for MinGW-w64 d4999d40b9

test: Add fs_tests/rename unit test

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

dc01cbc538

hebasto force-pushed on Feb 14, 2022

hebasto commented at 2:58 pm on February 14, 2022: member

Updated 4d92107041c508cf7bca47dce088979e519fb8bc -> dc01cbc538765f64326bca30952c83e3862d0d54 (pr24331.02 -> pr24331.03, diff):

addressed @laanwj’s comment

vasild approved

vasild commented at 3:12 pm on February 14, 2022: member

ACK dc01cbc538765f64326bca30952c83e3862d0d54

DrahtBot commented at 1:55 am on February 15, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24338 (util: Work around libstdc++ create_directories issue by laanwj)
#24169 (build: Add –enable-c++20 option by MarcoFalke)

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 merged this on Feb 17, 2022

laanwj closed this on Feb 17, 2022

hebasto deleted the branch on Feb 17, 2022

laanwj commented at 11:50 am on February 17, 2022: member

This is still unclear to me?

FWIW there’s a comment in the source

0    // This bug has been fixed in upstream:
1    //  - GCC 10.3: 8dd1c1085587c9f8a21bb5e588dfe1e8cdbba79e
2    //  - GCC 11.1: 1dfd95f0a0ca1d9e6cbc00e6cbfd1fa20a98f312

sidhujag referenced this in commit aa1d6e2c06 on Feb 18, 2022

DrahtBot locked this on Feb 17, 2023

util: Revert back MoveFileExW call for MinGW-w64 #24331

Conflicts

util: Revert back `MoveFileExW` call for MinGW-w64 #24331