Page MenuHomePhabricator

Merge #10899: [test] Qt: Use _putenv_s instead of setenv on Windows builds
AbandonedPublic

Authored by jasonbcox on Jun 5 2019, 22:49.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

0be03c7 Qt: Use _putenv_s instead of setenv on Windows builds (Brian McMichael)

Pull request description:

Fixes https://github.com/bitcoin/bitcoin/issues/10836

Error message I would get on `make`:
```
...
  CXXLD    bench/bench_bitcoin.exe
  OBJCXXLD qt/bitcoin-qt.exe
qt/test/test_main.cpp: In function ‘int main(int, char**)’:
qt/test/test_main.cpp:64:43: error: ‘setenv’ was not declared in this scope
     setenv("QT_QPA_PLATFORM", "minimal", 0);
                                           ^
make[2]: *** [qt/test/qt_test_test_bitcoin_qt-test_main.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/bmcmichael/Projects/bcoin/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/bmcmichael/Projects/bcoin/src'
make: *** [all-recursive] Error 1
```

`setenv` function is not available from the Microsoft runtime library. Need to use `_putenv_s` instead.

This solution tells the compiler to use `_putenv_s` on `WIN32` compilation (Note: this also works on 64-bit Windows instances.) and `setenv` everywhere else.

I've tested builds on Windows 10 x64 and Ubuntu 16.04 with this code.

Tree-SHA512: d53c996c890e3c6f22b4f2dcca718bef9168f19a6d4a29b8ff13391bfc0c8ea9c1cd16782b47c25b156dcbdff18bb19e23bfd5f6fefb1f373c9d5454a13fc969

Backport of Core PR10899
https://github.com/bitcoin/bitcoin/pull/10899/files

This backport was attempted once, and issues were identified with it (see D1368 for details).
However, given our change in strategy to backport as much as possible and not take code ownership
where it isn't necessary, we should re-evaluate this diff.

Test Plan
make check
# I don't have a Windows dev machine to test this on.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr10899
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6173
Build 10394: Bitcoin ABC Buildbot (legacy)
Build 10393: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jun 5 2019, 22:49
Fabien requested changes to this revision.Jun 6 2019, 10:43
Fabien added a subscriber: Fabien.

This is fixing something which is not broken, maybe due to different MinGw versions.
Furthermore I need to investigate, because the qt tests are failing on Windows with the minimal platform. You have to specify test_bitcoin-qt.exe -platform windows to make them green.
This will eventually end up being removed.

This revision now requires changes to proceed.Jun 6 2019, 10:43
Fabien added a comment.Jun 6 2019, 10:52

NVM the "crash" is documented in src/st/test/wallettests.cpp:

// This also requires overriding the default minimal Qt platform:
//
//     src/qt/test/test_bitcoin-qt -platform xcb      # Linux
//     src/qt/test/test_bitcoin-qt -platform windows  # Windows
//     src/qt/test/test_bitcoin-qt -platform cocoa    # macOS

I still think this backport is not necessary, as it provides no value and makes the code worst.

deadalnix accepted this revision.Jun 6 2019, 17:21
jasonbcox abandoned this revision.Jul 2 2019, 16:24

Abandoning this for now since the value is questionable.