-
Notifications
You must be signed in to change notification settings - Fork 578
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
add cmake support for Tpetra_ENABLE_DEPRECATED_CODE #3742
Conversation
…inal template parameters
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_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Using Repos:
Pull Request Author: GeoffDanielson |
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.
Thanks for volunteering for this task, @GeoffDanielson
Two comments/questions:
- Does this change make the #define value Tpetra_DISABLE_DEPRECATED_CODE into the source code? I expected to see a change to tpetra/core/cmake/TpetraCore_configs.h.in, adding a cmakedefine option there. I am not a Cmake wizard, so I would have followed the pattern of some other Tpetra option (e.g., Tpetra_INST_FLOAT).
- This flag will be used for all deprecated code (e.g., for your work with @tjfulle deprecating DynamicProfile), not only for the template deprecation. Thus, more general MESSAGE STATUS text would be preferred.
BTW: If you want the learning experience of adding new tests to Tpetra, you could write a simple one that CMake enables only if the new CMake option is enabled and does something like
For when the CMake option isn't set, you could have a test with the opposite behavior. |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
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.
Thanks for working on this! I've left a comment explaining how to define a new CMake option. Also, in order for the macro to exist, you must add the following line to Trilinos/packages/tpetra/core/cmake/TpetraCore_config.h.in
:
#cmakedefine TPETRA_DISABLE_DEPRECATED_CODE
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
I just marked this as a WIP, mainly so that it doesn't clog the testing pipeline, since we know what changes it needs. |
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.
Some possibly incorrect CMake syntax, and suggestions for changes to docstrings and comments. Thanks!
packages/tpetra/CMakeLists.txt
Outdated
MESSAGE (STATUS " - Tpetra_DISABLE_DEPRECATED_CODE is OFF, so Tpetra will assume you want the old template behavior.") | ||
ENDIF() | ||
SET (Tpetra_ENABLE_DEPRECATED_CODE_DEFAULT ON) | ||
MESSAGE (STATUS "Determine whether Tpetra will enable ordinal template arguments and dynamic allocation. By default, this will be ON.") |
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.
Please just specify a general message here. "Whether Tpetra enables deprecated code at compile time. Default is ON (deprecated code enabled)." We plan on using this macro in the future, for deprecating other things that we inevitably will want to deprecate :-) . This also imitates Kokkos, which uses its macro in a generic way, for all deprecated features.
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.
Got it. Thanks
packages/tpetra/core/CMakeLists.txt
Outdated
Tpetra_ENABLE_DEPRECATED_CODE | ||
TPETRA_ENABLE_DEPRECATED_CODE | ||
"Disable Tpetra deprecated code (templated ordinal types and dynamic graph/matrix allocation)" | ||
Tpetra_ENABLE_DEPRECATED_CODE_DEFAULT |
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.
I think you need to dereference the default value here. Do this:
${Tpetra_ENABLE_DEPRECATED_CODE_DEFAULT}
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.
You're right, I did need that for the default.
@@ -2,6 +2,9 @@ | |||
#define TPETRACORE_CONFIG_H | |||
/* CMake uses this file to generate TpetraCore_config.h automatically */ | |||
|
|||
/* define if user is using the deprecated codebase (templated ordinals, dynamicprofile) or not */ |
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.
See above. "Deprecated code" is a generic thing; we should not refer to specifically deprecated features here.
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.
Changed.
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.
Thanks for the fixes! :-D
@kddevin It looks like Geoff addressed your concerns; shall we go ahead with the merge? |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
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_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: GeoffDanielson |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ kddevin mhoemmen ]! |
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
Merge on Pull Request# 3742: IS A SUCCESS - Pull Request successfully merged |
This adds support to the CMakeLists.txt file to allow a macro switch to disable the old-style ordinal template args and dynamic allocation on matrices and graphs
@trilinos/tpetra
Description
Added a default value to the tpetra CMakeLists.txt file, support for the switch to the tpetra/src/core CMakeLists.txt, and a cmakedefine to TpetraCore_config.h.in
Motivation and Context
This is an effort toward removing the template arguments (local/global/index ordinal types) and dynamic profile support from tpetra without breaking backward compatibility with current apps
How Has This Been Tested?
This was tested by running CMake with -DTpetra_ENABLE_DEPRECATED_CODE explicitly included in both (ON|OFF) states, and excluded to make sure that CMake built appropriately.
Checklist