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.

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

E2E tests require Python 3.7 #10327

Closed
AlexeySachkov opened this issue Jul 12, 2023 · 1 comment
Closed

E2E tests require Python 3.7 #10327

AlexeySachkov opened this issue Jul 12, 2023 · 1 comment
Labels
bug Something isn't working confirmed

Comments

@AlexeySachkov
Copy link
Contributor

There is an inconsistency about minimum required python version: we have an implicit requirement for 3.7, whilst our explicit requirement is only 3.6

Explicit requirement is documented here:

llvm/llvm/CMakeLists.txt

Lines 839 to 842 in e753fae

if(LLVM_INCLUDE_TESTS)
# Lit test suite requires at least python 3.6
set(LLVM_MINIMUM_PYTHON_VERSION 3.6)
else()

Implicit requirement is caused by the following lines:

sp = subprocess.run((cmd), env=llvm_config.config.environment,
shell=True, capture_output=True, text=True)

capture_output argument of subprocess.run does not exists in Python 3,6 and it is only added in Python 3.7:

Changed in version 3.7: Added the text parameter, as a more understandable alias of universal_newlines. Added the capture_output parameter.

stackoverflow suggests that we could use check_output function instead.

Alternatively, we could have raised our minimum required Python version to 3.7, but I don't think that it is a good idea, unless upstream LLVM does so.

@aelovikov-intel: FYI

@AlexeySachkov AlexeySachkov added the bug Something isn't working label Jul 12, 2023
@github-actions github-actions bot added the Stale label Feb 1, 2024
@intel intel deleted a comment from github-actions bot Feb 1, 2024
@AlexeySachkov
Copy link
Contributor Author

#16211 made me realize that our requirements have changed since then and 3.8 is a minimum required version now:

llvm/llvm/CMakeLists.txt

Lines 979 to 982 in 9b9e5de

if(LLVM_INCLUDE_TESTS)
# All LLVM Python files should be compatible down to this minimum version.
set(LLVM_MINIMUM_PYTHON_VERSION 3.8)
else()

Therefore, this is not an issue anymore

@AlexeySachkov AlexeySachkov closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working confirmed
Projects
None yet
Development

No branches or pull requests

2 participants