Fix Windows build with --enable-werror by hebasto · Pull Request #20586 · bitcoin/bitcoin

hebasto · 2020-12-06T22:02:55Z

This PR makes possible to cross-compile Windows build with --enable-werror --enable-suppress-external-warnings.
Some problems are fixed, others are silenced.

Also --enable-werror is enabled for Cirrus CI Windows build (the last one on Cirrus CI without --enable-werror).

hebasto · 2020-12-06T22:03:38Z

cc @fanquake @MarcoFalke @vasild

DrahtBot · 2020-12-06T22:48:40Z

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

Conflicts

No conflicts as of last run.

src/fs.cpp

configure.ac

laanwj · 2020-12-07T12:52:38Z

I'm a bit divided on whether building on every platform/compiler combination 'with -Werror' is actually a goal of the project. I mean, I think this is impossible, and will result in a lot of warning fixing for the sake of warning fixing.

In general Werror is a curse.

maflcko · 2020-12-07T13:06:41Z

Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.

vasild

ACK 06ff475

Sometimes important warnings are emitted only on some platforms, so I think it is good to use -Werror on as most platforms as possible.

If keeping Windows builds warning-free turns out to be too big burden then the NO_WERROR=1 can be added back.

hebasto · 2020-12-07T13:11:14Z

I'm a bit divided on whether building on every platform/compiler combination 'with -Werror' is actually a goal of the project. I mean, I think this is impossible, and will result in a lot of warning fixing for the sake of warning fixing.

In general Werror is a curse.

Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.

Fair enough.

The goal of this PR (combined with #20544) is to prevent changes that introduce new warnings.

Anyway, feel free to close this.

DrahtBot · 2020-12-09T16:09:33Z

🕵️ @practicalswift has been requested to review this pull request as specified in the REVIEWERS file.

practicalswift · 2020-12-09T16:22:01Z

Concept ACK

hebasto · 2020-12-10T12:06:56Z

Updated 06ff475 -> 926f6d8 (pr20586.01 -> pr20586.02, diff).

@MarcoFalke

Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.

Only one-line change in fs.cpp now.

src/fs.cpp

vasild

ACK 926f6d8

luke-jr · 2020-12-14T01:03:48Z

src/fs.cpp

 static std::string openmodeToStr(std::ios_base::openmode mode)
 {
-    switch (mode & ~std::ios_base::ate) {
+    switch (static_cast<int>(mode & ~std::ios_base::ate)) { // casting is required to suppress -Wswitch warnings


NACK, the standard doesn't guarantee this fits in an int...

Can you use a pragma to disable the warning here?

hebasto · 2020-12-19T17:10:55Z

Reverted back 06ff475 <- 926f6d8 (pr20586.01 <- pr20586.02) as was requested by @luke-jr.

This variant has been already ACKed by @vasild.

practicalswift · 2021-03-06T07:37:40Z

cr ACK 06ff475: patch looks correct

FWIW, having this in master would have prevented the issue #21355 pre-merge :)

src/fs.cpp

hebasto · 2021-04-13T07:25:42Z

Updated 06ff475 -> b367745 (pr20586.01 -> pr20586.03):

rebased due to the conflict with Move external signer out of wallet module #21467
addressed @MarcoFalke's comment

vasild

ACK b367745

jarolrod

ACK b367745

Tested on Linux cross-compiling to Windows passing --enable-werror to configure

Master:

script/standard.cpp:215:26: error: control reaches end of non-void function [-Werror=return-type]
  215 |     std::vector<valtype> vSolutions;

PR:
Builds fine and built binaries work fine on Windows 10

practicalswift · 2021-06-06T11:09:45Z

cr ACK b367745: patch looks correct

hebasto · 2021-06-17T16:02:56Z

@laanwj @MarcoFalke @luke-jr

Do you mind having another look at this PR?

maflcko · 2021-06-23T08:41:48Z

src/fs.cpp

-
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wswitch"


Will this be needed after the boost::fs removal?

I think no, as this bunch of code should go:

bitcoin/src/fs.h

Lines 66 to 69 in d6e0d78

// Work around this issue by creating stream objects with `_wfopen` in

// combination with `__gnu_cxx::stdio_filebuf`. This workaround can be removed

// with an upgrade to C++17, where streams can be constructed directly from

// `std::filesystem::path` objects.

laanwj · 2021-08-27T06:20:40Z

I'm not a big fan in general of pragmas or fixing warnings for the sake of fixing warnings but this seems minimal enough.
Code review ACK b367745

DrahtBot added Build system GUI labels Dec 6, 2020

This was referenced Dec 6, 2020

build: Do not repeat warning names in -Werror=... options #20544

Closed

Replace boost::filesystem with std::filesystem #19245

Closed

maflcko removed the GUI label Dec 7, 2020

vasild reviewed Dec 7, 2020

View reviewed changes

src/fs.cpp Show resolved Hide resolved

configure.ac Show resolved Hide resolved

vasild approved these changes Dec 7, 2020

View reviewed changes

hebasto force-pushed the 201206-win branch from 06ff475 to 926f6d8 Compare December 10, 2020 12:03

vasild reviewed Dec 10, 2020

View reviewed changes

src/fs.cpp Outdated Show resolved Hide resolved

vasild approved these changes Dec 10, 2020

View reviewed changes

luke-jr suggested changes Dec 14, 2020

View reviewed changes

hebasto force-pushed the 201206-win branch from 926f6d8 to 06ff475 Compare December 19, 2020 17:06

DrahtBot mentioned this pull request Dec 22, 2020

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

Merged

DrahtBot mentioned this pull request Feb 2, 2021

build: build fuzz tests by default #20936

Merged

DrahtBot added the Needs rebase label Feb 8, 2021

DrahtBot removed the Needs rebase label Feb 19, 2021

hebasto mentioned this pull request Mar 4, 2021

windows: new -Wreturn-type warnings after #19203 #21355

Closed

maflcko reviewed Mar 6, 2021

View reviewed changes

src/fs.cpp Show resolved Hide resolved

This was referenced Mar 17, 2021

build: Add convenient BITCOIN_TRY_ADD_COMPILE_FLAG macro #21459

Closed

build: Add -Werror=unused... compile flags #21458

Closed

DrahtBot mentioned this pull request Mar 18, 2021

Move external signer out of wallet module #21467

Merged

DrahtBot mentioned this pull request Apr 3, 2021

rpc, gui: bumpfee signer support #21576

Merged

hebasto added 2 commits April 13, 2021 10:20

Fix Windows build with --enable-werror on Ubuntu Focal

c713bb2

ci: Make Cirrus CI Windows build with --enable-werror

b367745

hebasto force-pushed the 201206-win branch from 06ff475 to b367745 Compare April 13, 2021 07:22

vasild approved these changes Apr 13, 2021

View reviewed changes

DrahtBot mentioned this pull request Apr 13, 2021

build: Add -Werror=missing-noreturn #21667

Closed

DrahtBot mentioned this pull request May 8, 2021

Add minisketch subtree and integrate in build/test #21859

Closed

jarolrod reviewed Jun 6, 2021

View reviewed changes

maflcko reviewed Jun 23, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 24, 2021

[TESTBED][NO-MERGE][POC] Use std::filesystem. Remove Boost Filesystem & System #22336

Closed

fanquake mentioned this pull request Jul 18, 2021

build: fix -Wreturn-type compiler warnings (Win64) #22484

Closed

laanwj merged commit 19364c0 into bitcoin:master Aug 27, 2021

hebasto deleted the 201206-win branch August 27, 2021 06:46

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 28, 2021

Merge bitcoin#20586: Fix Windows build with --enable-werror

4fe76c8

str4d mentioned this pull request Dec 6, 2021

Backport changes to src/fs.{cpp,h} zcash/zcash#5406

Open

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

	// Work around this issue by creating stream objects with `_wfopen` in
	// combination with `__gnu_cxx::stdio_filebuf`. This workaround can be removed
	// with an upgrade to C++17, where streams can be constructed directly from
	// `std::filesystem::path` objects.

Conversation

hebasto commented Dec 6, 2020

Uh oh!

hebasto commented Dec 6, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

laanwj commented Dec 7, 2020

Uh oh!

maflcko commented Dec 7, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 7, 2020

Uh oh!

DrahtBot commented Dec 9, 2020

Uh oh!

practicalswift commented Dec 9, 2020

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 19, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 19, 2020

Uh oh!

practicalswift commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hebasto commented Apr 13, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 6, 2021

Uh oh!

hebasto commented Jun 17, 2021

Uh oh!

maflcko Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Dec 6, 2020 •

edited

Loading

hebasto commented Dec 10, 2020 •

edited

Loading

practicalswift commented Mar 6, 2021 •

edited

Loading