Standardize all environment variable boolean configuration in python's setup.py#25444
Standardize all environment variable boolean configuration in python's setup.py#25444jtattermusch merged 5 commits intogrpc:masterfrom
Conversation
b71e7bd to
69ebb46
Compare
lidizheng
left a comment
There was a problem hiding this comment.
Thanks for putting up this PR!
setup.py
Outdated
| asm_key = '' | ||
| if BUILD_WITH_BORING_SSL_ASM and not BUILD_WITH_SYSTEM_OPENSSL: | ||
| if (BUILD_WITH_BORING_SSL_ASM.upper() == 'TRUE' | ||
| or BUILD_WITH_BORING_SSL_ASM == '1') and not BUILD_WITH_SYSTEM_OPENSSL: |
There was a problem hiding this comment.
nit: Technically, users can set this flag to any value and they should be evaluated into True, except False, 0, ''. How about we detect the falsify values intead?
jtattermusch
left a comment
There was a problem hiding this comment.
Thanks for the contribution but I think we really need to make all the boolean build options behave consistently, see comment
setup.py
Outdated
|
|
||
| asm_key = '' | ||
| if BUILD_WITH_BORING_SSL_ASM and not BUILD_WITH_SYSTEM_OPENSSL: | ||
| if (BUILD_WITH_BORING_SSL_ASM.upper() not in ['FALSE', '0', ''] and |
There was a problem hiding this comment.
Fair enough, but as pointed out in #24498 (comment) there are plenty of other options that can be set for setup.py and by changing the behavior for BUILD_WITH_BORING_SSL_ASM only, we're basically introducing an inconsistency (and having some setup.py with one behavior and some with a different behavior is something we really don't want).
My suggestion is to introduce a private function e.g. _get_build_option_bool_value(env_name)) that has the upper() not in ['FALSE', '0', ''] logic in it and then use it for all the boolean build options in setup.py - there are quite a few.
|
Also not that the sanity check is failing (yapf code formatting): |
|
@jtattermusch I tried to address you comments. Would you mind taking another look? Also how can I run lint/formatting locally (I can't seem to find the instructions, which I feel like I had at one point). |
jtattermusch
left a comment
There was a problem hiding this comment.
LGTM once the tests pass.
|
Needed to rebase. |
|
The interop test failure is #25652. |
@jtattermusch @lidizheng
Fixes #24498