Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project
Restricted Owners Package	(Owns No Changed Paths)

Summary

Fixes QT test compiliation on Windows builds

Test Plan

src/test/test_bitcoin --run_test=util_tests/util_EnvironmentalVariables

Diff Detail

Repository

rABC Bitcoin ABC

Branch

arcpatch-D1368

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3417
Build 4910: Bitcoin ABC Buildbot (legacy)
Build 4909: arc lint + arc unit

Event Timeline

movrcx created this revision.May 3 2018, 18:12

Owners added a reviewer: Restricted Owners Package.May 3 2018, 18:12

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMay 3 2018, 18:12

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2494: Diff 3754.May 3 2018, 19:01

Thanks for the fix! :)

This revision is now accepted and ready to land.May 3 2018, 21:45

What problem is it fixing ?

Chatted with Amaury and this definitely needs a test. There's nothing keeping this from silently breaking again.

This revision now requires changes to proceed.May 4 2018, 14:54

Googleing a bit, it seems like setenv doesn't exist on windows. So:

There is no way to to know if the fix is appropriate with knowing what the problem is.
If the problem is that there is no way to set environment variable in a plateform agnostic manner, and we want one, then we make one.
In the absence of the above mentionned way, it is only a matter of time before this breaks again.
It doesn't seem to be the responsibility of the main function to know about the management of environment variable on various systems so it seems inappropriate to put it there.
It doesn't looks like the cross compilation has been tested in any way. I suspect it may not be working at all.

In D1368#27524, @deadalnix wrote:

Googleing a bit, it seems like setenv doesn't exist on windows. So:

There is no way to to know if the fix is appropriate with knowing what the problem is.

If the problem is that there is no way to set environment variable in a plateform agnostic manner, and we want one, then we make one.

In the absence of the above mentionned way, it is only a matter of time before this breaks again.

It doesn't seem to be the responsibility of the main function to know about the management of environment variable on various systems so it seems inappropriate to put it there.

It doesn't looks like the cross compilation has been tested in any way. I suspect it may not be working at all.

This is a backport from https://github.com/bitcoin/bitcoin/commit/0be03c71bd91db25f30bcf4d45facf1e26dcfd30#diff-26db92d97df78fb26609428cb0aaee6e and setting the envar only happens here. Seems a bit overkill to make a brand new function to set it but if that's whats needed then I'm happy to build a function for it.

This is still a WIP, going to make this an independent function and add tests...

Add platform-independent SetEnvironmentalVariable() and GetEnvironmentalVariable() functions

movrcx retitled this revision from Qt: Use _putenv_s instead of setenv on Windows builds to Add platform-independent SetEnvironmentalVariable() and GetEnvironmentalVariable() functions.May 7 2018, 17:40

movrcx edited the test plan for this revision. (Show Details)

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B2535: Diff 3803!May 7 2018, 17:43

All the modules related to compatibility are in compat and pretty much nothing uses setenv so I'm not sure what this lands in util. It seems like there is enough garbage in there.

Also considering calls to setenv were missed, it's fair to assume a better approach is to actually define setenv on windows so no code change is required. This is what libtool is doing for instance.

src/qt/test/test_main.cpp
64 ↗	(On Diff #3803)	Semantic is different now.
src/util.cpp
603 ↗	(On Diff #3803)	There is a setenv call int hat very file.
649 ↗	(On Diff #3803)	The two codepath do something different.
657 ↗	(On Diff #3803)	Not sure what this brings if this is not plateform dependent.

This revision now requires changes to proceed.May 7 2018, 18:22

movrcx added inline comments.May 7 2018, 18:31

src/util.cpp
603 ↗	(On Diff #3803)	This will be updated to use SetEnvironmentalVariable()
649 ↗	(On Diff #3803)	The _putenv_s function adds new environment variables or modifies the values of existing environment variables. Environment variables define the environment in which a process executes (for example, the default search path for libraries to be linked with a program). _wputenv_s is a wide-character version of _putenv_s; the envstring argument to _wputenv_s is a wide-character string. The setenv() function shall update or add a variable in the environment of the calling process. The envname argument points to a string containing the name of an environment variable to be added or altered. The environment variable shall be set to the value to which envval points. The function shall fail if envname points to a string which contains an '=' character. If the environment variable named by envname already exists and the value of overwrite is non-zero, the function shall return success and the environment shall be updated. I will switch the overwrite flag to 1 so the behavior is similar.
657 ↗	(On Diff #3803)	I'll remove this function.