-
Notifications
You must be signed in to change notification settings - Fork 51
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
Remove deprecations: tock #1091
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Carlos Agüero <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Carlos Agüero <[email protected]>
@@ -151,8 +151,6 @@ endif() | |||
set(GZ_RENDERING_ENGINE_RELATIVE_INSTALL_DIR | |||
${GZ_LIB_INSTALL_DIR}/gz-${GZ_DESIGNATION}-${PROJECT_VERSION_MAJOR}/engine-plugins | |||
) | |||
set(GZ_RENDERING_ENGINE_INSTALL_DIR | |||
${CMAKE_INSTALL_PREFIX}/${GZ_RENDERING_ENGINE_RELATIVE_INSTALL_DIR}) |
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 we may still need to keep this cmake variable here as I see that these are being used in ogre/src/CMakeLists.txt and ogre2/src/CMakeLists.txt
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 we replace these calls with getResourcePath()
?
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.
getResourcePath()
is a C++ function so we won't be able to replace those calls in CMakeLists.txt. I think we can just keep this cmake var here. Other changes in this PR are fine.
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.
Sorry I meant GZ_RENDERING_RESOURCE_PATH
, that it's used in some .cc
files (although also in optix/src/CMakeLists.txt
).
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.
oh do you see the macro used in cc files? For example, I see usage like this that it just gets the value directly from the GZ_RENDERING_RESOURCE_PATH
environment variable (instead of the macro from config.hh).
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.
to clarify, I think we are still keeping the environment variable and only deprecating the macro? Or are we deprecating / removing both?
🦟 Bug fix
Perform the tock of the tick-tock cycle and remove / move deprecated code.
Summary
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.