Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Standardize all environment variable boolean configuration in python's setup.py #25444

Merged
merged 5 commits into from
Mar 10, 2021
Merged

Standardize all environment variable boolean configuration in python's setup.py #25444

merged 5 commits into from
Mar 10, 2021

Conversation

emkornfield
Copy link
Contributor

@emkornfield emkornfield commented Feb 12, 2021 •
edited by jtattermusch
Loading

lidizheng
lidizheng reviewed Feb 12, 2021
View reviewed changes
Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting up this PR!

setup.py Outdated
@@ -300,7 +300,8 @@ def check_linker_need_libatomic():
asm_files = []

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@lidizheng lidizheng added lang/Python release notes: yes Indicates if PR needs to be in release notes labels Feb 12, 2021
lidizheng
lidizheng approved these changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

jtattermusch
jtattermusch requested changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution but I think we really need to make all the boolean build options behave consistently, see comment

setup.py Outdated
@@ -300,7 +300,8 @@ def check_linker_need_libatomic():
asm_files = []

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jtattermusch
Copy link
Contributor

Also not that the sanity check is failing (yapf code formatting):
https://source.cloud.google.com/results/invocations/2c0a0875-171c-4b32-bba1-35b38964b206/targets/github%2Fgrpc%2Frun_tests%2Fsanity_linux_dbg_native%2Ftools%2Fdistrib%2Fyapf_code.sh/tests

@lidizheng lidizheng mentioned this pull request Feb 16, 2021
Merged
@emkornfield
Copy link
Contributor Author

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

@emkornfield emkornfield changed the title Use string comparison for BUILD_WITH_BORING_SSL_ASM Standardize all environment variable boolean configuration Mar 4, 2021
jtattermusch
jtattermusch approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the tests pass.

@emkornfield
Copy link
Contributor Author

Needed to rebase.

@jtattermusch
Copy link
Contributor

The interop test failure is #25652.

@jtattermusch jtattermusch changed the title Standardize all environment variable boolean configuration Standardize all environment variable boolean configuration in python's setup.py Mar 9, 2021
@jtattermusch jtattermusch merged commit ec31fa8 into grpc:master Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtattermusch jtattermusch jtattermusch approved these changes

@lidizheng lidizheng Awaiting requested review from lidizheng

Assignees

@lidizheng lidizheng

Labels
lang/Python release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reading of options from env variables is broken in setup.py (e.g. BUILD_WITH_BORING_SSL_ASM)
4 participants
@emkornfield @jtattermusch @lidizheng @grpc-kokoro