-
Notifications
You must be signed in to change notification settings - Fork 572
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
Framework: Conditionally warn about including header for deprecated package #12828
Conversation
If <packagename>_SHOW_DEPRECATED_WARNINGS is defined, emit a warning upon inclusion of any package header (used Epetra for an example).
Wile I think this is a workable solution from SIERRA's perspective, I still strongly advocate for deprecating the interface with the Why deprecation attributes are preferred to header-level deprecation While there is a single warning per include of deprecated headers, that may be several layers of includes deep and far from usage of the deprecated interface. That distance from warning to actual use makes it difficult to track down, and may induce several false positives due to transitive includes in user code. Past experience suggests that these warnings will be largely ignored, whereas local deprecation warnings/errors are much more likely to be addressed in a timely manner. SIERRA will effectively need to set |
I agree with @mdmosby that it is preferrable to deprecate individual classes and functions using I am curious about the requirements for this deprecation? I don't see those listed anywhere. |
edc2422
to
8a7dfa4
Compare
Force pushed as the commits adding deprecation warnings to Amesos and AztecOO did not have |
FYI: I just viewed a presentation yesterday that describes how to write automated clang-tidy checks/refractors and they mentioned they are using this to add deprecation attributes to C++ entities. Here is the presentation: She mentions an example at Google where they are using clang-tidy custom checks/refactors to add deprecated checks. Looks like Kitware has some expertise with this: It looks like CMake 3.26 also added a feature to help apply automated refactorings. SNL should really learn about how to write custom static analysis and refactoring. This could be a real benefit to Trilinos and many other internal projects. |
Can I double 👍 this? haha |
These conditional warnings depend on when Thyra_SHOW_DEPRECATED_WARNINGS is defined instead of ThyraEpetraAdapters_SHOW_DEPRECATED_WARNINGS as I could not get the ThyraEpetraAdapters_SHOW_DEPRECATED_WARNINGS defined properly. Since Thyra Core is depended upon anyways, this seemed appropriate enough and works when configuring with ThyraEpetraAdapters enabled. PR: trilinos#12828
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. I guess if someone is still using the old packages, they will be faced with a deluge of warnings :-)
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ cgcgcg ]! |
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
1 similar comment
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
Added a RELEASE_NOTES entry, need new review. |
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.20-uvm
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ jmlapre ]! |
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
Added `-Werror=sign-compare -Werror=unused-variable -Werror=parentheses` to the rhel8_gcc-8.5.0-serial Trilinos configuration. These three warnings-as-errors flags serve as the starting steps to introduce more warnings-as-errors flags going forward without overwhelming package developers too much off the bat. NOTE: Trilinos cannot currently enable `-Werror` due to how deprecated package header warnings are implemented in PR trilinos#12828 (controlled by -Wcpp which is controls a lot of other warnings). Signed-off-by: Anderson <[email protected]>
If
<packagename>_SHOW_DEPRECATED_WARNINGS
is defined, emit a warning upon inclusion of any package header (used Epetra for an example).@csiefer2 @cgcgcg @ccober6 what do you think about this approach? I thought about it after discussing with Matt and came up with this change, which still works around the function and class deprecation flags.
Edit: Tagging @bartlettroscoe as well in case he has any input on any alternative ways to "deprecate a package".
Now here are the states:
cmake -DTrilinos_ENABLE_Epetra=ON -DTrilinos_ENABLE_ALL_OPTIONAL_PACKAGES=OFF -DTrilinos_SHOW_DEPRECATED_WARNINGS=ON ..
gives me compile warnings (default)cmake -DTrilinos_ENABLE_Epetra=ON -DTrilinos_ENABLE_ALL_OPTIONAL_PACKAGES=OFF -DTrilinos_SHOW_DEPRECATED_WARNINGS=OFF ..
gives me no warnings or errorscmake -DTrilinos_ENABLE_Epetra=ON -DTrilinos_ENABLE_ALL_OPTIONAL_PACKAGES=OFF -DTrilinos_HIDE_DEPRECATED_CODE=ON ..
gives me configure failures (one cannot even configure Trilinos if hiding deprecated code and Epetra being enabled).Packages to add deprecation warning to headers