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
BLD Avoid misleading OpenMP warning with Apple Clang #28839
BLD Avoid misleading OpenMP warning with Apple Clang #28839
Conversation
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.
+1 to disable the misleading warning until it's fixed in meson.
sklearn/meson.build
Outdated
''' | ||
import os | ||
|
||
compiler_env_vars_to_check = ["CPPFLAGS", "CFLAGS", "CXXFLAGS"] | ||
|
||
compiler_env_vars_with_openmp = [var for var in compiler_env_vars_to_check if "-fopenmp" in os.getenv(var, "")] | ||
print(compiler_env_vars_with_openmp) |
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.
can you put this in a variable to make the run_command
invoke more readable ?
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.
LGTM
var for var in compiler_env_vars_to_check if "-fopenmp" in os.getenv(var, "")] | ||
print(compiler_env_vars_with_openmp) | ||
'''], check: true).stdout().strip() | ||
warn_about_missing_openmp = compiler_env_vars_with_openmp == '[]' |
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 would be more standard I guess if the process would return 1 on failure instead of checking program's output. But it doesn't really matter here I guess.
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 not but at the same time, the process returning 1 could also be because there is a syntax error in the Python snippet, so you would need additional logic or a different exit code, which would complicate things a bit.
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.
although, you could return a non-1 value. But doesn't matter. Merged :)
Close #28710.
For now Meson OpenMP detection can not be trusted on Apple Clang, so when compilers environment variables are set we assume everything is going to be fine and the warning can be less scary.
I tested it on a OSX VM, it seems to work fine. I will trigger a wheel build since the macOS wheels should show the milder warning.
Edit: it does build log