Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 17, 2022
Merged

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

merged 2 commits into from
Feb 17, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 13, 2022

Unfortunately, #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.

@fanquake
Copy link
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?

@maflcko
Copy link
Member

maflcko commented Feb 13, 2022

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.

@maflcko
Copy link
Member

maflcko commented Feb 13, 2022

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

Concept ACK

@laanwj
Copy link
Member

laanwj commented Feb 14, 2022

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

@maflcko
Copy link
Member

maflcko commented Feb 14, 2022

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.

laanwj
laanwj reviewed Feb 14, 2022
View reviewed changes
@laanwj
Copy link
Member

laanwj commented Feb 14, 2022 •
edited
Loading

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.

vasild
vasild approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 15a043d

@vasild
Copy link
Contributor

vasild commented Feb 14, 2022

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

@hebasto
Copy link
Member Author

hebasto commented Feb 14, 2022

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

The latter, I guess.

@hebasto
Copy link
Member Author

hebasto commented Feb 14, 2022

Updated 15a043d -> 4d92107 (pr24331.01 -> pr24331.02, diff):

vasild
vasild approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4d92107

@fanquake
Copy link
Member

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

@laanwj
Copy link
Member

laanwj commented Feb 14, 2022 •
edited
Loading

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
Copy link
Member Author

hebasto commented Feb 14, 2022

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
Copy link
Member Author

hebasto commented Feb 14, 2022

Updated 4d92107 -> dc01cbc (pr24331.02 -> pr24331.03, diff):

vasild
vasild approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dc01cbc

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 15, 2022 •
edited
Loading

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.

This was referenced Feb 15, 2022
Merged
Merged
@laanwj laanwj merged commit df08250 into bitcoin:master Feb 17, 2022
@hebasto hebasto deleted the 220213-rename branch February 17, 2022 11:32
@laanwj
Copy link
Member

laanwj commented Feb 17, 2022

This is still unclear to me?

FWIW there's a comment in the source

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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 18, 2022
@laanwj
Merge bitcoin#24331: util: Revert back MoveFileExW call for MinGW-w64
aa1d6e2 
dc01cbc test: Add fs_tests/rename unit test (Hennadii Stepanov)
d4999d4 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov)

Pull request description:

  Unfortunately, bitcoin#24308 introduced a [regression](bitcoin#24308 (comment)) for mingw builds.

  The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/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](bitcoin#24308 (review)) unit test.

ACKs for top commit:
  vasild:
    ACK dc01cbc

Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
@bitcoin bitcoin locked and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @fanquake @maflcko @laanwj @vasild @DrahtBot