-
Notifications
You must be signed in to change notification settings - Fork 341
🍒 [lldb] make lit use the same PYTHONHOME for building and testing (#143183) #10811
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.
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
🍒 [lldb] make lit use the same PYTHONHOME for building and testing (#143183) #10811
Conversation
…m#143183) When testing LLDB, we want to make sure to use the same Python as the one we used to build it. This patch used the CMake variable `Python3_ROOT_DIR` to set the `PYTHONHOME` env variable in LLDB lit tests, in order to ensure of this. Please see swiftlang/swift#82063 for the original issue.
@@ -25,6 +25,7 @@ set_default("gold_executable", "@GOLD_EXECUTABLE@") | |||
set_default("clang", "@COMPILER_RT_RESOLVED_TEST_COMPILER@") | |||
set_default("compiler_id", "@COMPILER_RT_TEST_COMPILER_ID@") | |||
set_default("python_executable", "@Python3_EXECUTABLE@") | |||
set_default("python_root_dir", "@Python3_ROOT_DIR@") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change in compiler-rt necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. I just checked by rerunning the tests without that setting. I thought this would set the default value for python_root_dir
globally to all lit tests but this is not the case.
I will open a PR on upstream to remove it and cherry-pick it back to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR opened: llvm#143738
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 👍
Should I squash the commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if they are cherry-picks.
…3738) In llvm#143183 was mistakenly added a default value to `python_root_dir` in lit tests of compiler-rt. This is unused by the lit tests of compiler-rt, as it was meant to be used by `lldb`. This patch removes this change.
@swift-ci please test |
@swift-ci please test macOS |
Uh oh!
There was an error while loading. Please reload this page.