Page MenuHomePhabricator

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

Authored by movrcx on May 3 2018, 18:12.

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
updateD1368
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3336
Build 4755: Bitcoin ABC Buildbot (legacy)
Build 4754: arc lint + arc unit

Event Timeline

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
jasonbcox added a subscriber: jasonbcox.

Thanks for the fix! :)

This revision is now accepted and ready to land.May 3 2018, 21:45
jasonbcox requested changes to this revision.May 4 2018, 14:54

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
deadalnix requested changes to this revision.May 4 2018, 15:31

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.

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)
deadalnix requested changes to this revision.May 7 2018, 18:22

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
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.

schancel added inline comments.
src/util.cpp
657 ↗(On Diff #3803)

Having consistent APIs are good. Please leave it.

Rebase off fresh master.

jasonbcox requested changes to this revision.Oct 1 2018, 21:54
jasonbcox added inline comments.
src/util.cpp
649 ↗(On Diff #3803)

Does this still need to be completed?

This revision now requires changes to proceed.Oct 1 2018, 21:54