-
Notifications
You must be signed in to change notification settings - Fork 217
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
Reproducer for PCA example failures (#3037) #3039
base: main
Are you sure you want to change the base?
Conversation
Looks like it didn't work. How about adding it to env. var |
/intelci: run |
.ci/scripts/test.sh
Outdated
@@ -221,6 +221,14 @@ for link_mode in "${link_modes[@]}"; do | |||
cmake_options+=(-DCMAKE_TOOLCHAIN_FILE="${ONEDAL_DIR}"/.ci/env/"${ARCH}"-"${compiler}"-crosscompile-toolchain.cmake) | |||
fi | |||
|
|||
if [ "$compiler" == "gnu" ] ; then |
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.
Looks like the flag is also supported by clang:
https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-copy
and by extension, should also be supported by icx. Perhaps it could be added unconditionally.
I do not like the idea of modifying environment variables. They could have impact on other processes besides cmake. |
Have you been able to reproduce this locally by adding the same flag? |
@david-cortes-intel |
I wasn't able to reproduce it either, neither with gcc nor clang nor icx. I notice the issue was raised with the "dynamic" variant of the examples. Just to double check: are the examples here compiled against the static or the dynamic-link library? |
@Vika-F On a deeper look, I was able to reproduce the error when using the exact same compiler version where the issue was reported (gcc==11.3). Since this happens only with GCC, and only with certain older versions, it leads me to believe that there might actually not be any issue in the code and we might instead be seeing a compiler bug like this: |
Both modes are tested here: static and dynamic. See this log, for example: https://dev.azure.com/daal/c48b0cee-5374-446e-aea4-3de8ba2ac9ca/_apis/build/builds/41717/logs/116 |
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.
While this didn't end up reproducing the original issue, it's still helpful to have this flag enabled anyway, so no harm in merging.
Add
-Werror=deprecated-copy
option to GCC examples build to reproduce the validation tests failures.PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance