-
Notifications
You must be signed in to change notification settings - Fork 269
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
Fix visibility macro names when used by a different component (Windows) #564
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.
Wow I didn't know those macros even existed! I'm still not sure if the plugins need the visibility to be set though.
Codechecker is sad - otherwise the PR looks good to me.
/github/workspace/include/ignition/gazebo/gui/Gui.hh:63: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/camera_video_recorder/CameraVideoRecorder.hh:44: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/joint_position_controller/JointPositionController.hh:75: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/logical_audio_sensor_plugin/LogicalAudioSensorPlugin.hh:132: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/multicopter_control/MulticopterVelocityControl.hh:149: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/multicopter_motor_model/MulticopterMotorModel.hh:37: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/optical_tactile_plugin/OpticalTactilePlugin.hh:69: Lines should be <= 80 characters long [whitespace/line_length] [2]
/github/workspace/src/systems/triggered_publisher/TriggeredPublisher.hh:157: Lines should be <= 80 characters long [whitespace/line_length] [2]
Total errors found: 8
Yeah, I don't think we want them to be visible. I would in fact vote for making them |
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
…ivero/win/fix_visibility_names
7d720f8
to
198d90a
Compare
Codecov Report
@@ Coverage Diff @@
## main #564 +/- ##
=======================================
Coverage 77.57% 77.57%
=======================================
Files 211 211
Lines 11582 11582
=======================================
Hits 8985 8985
Misses 2597 2597
Continue to review full report at Codecov.
|
I've removed visibility from all plugins.
I agree. I would leave the change of moving them to final classes in a different PR. |
…s) (#564) Signed-off-by: Jose Luis Rivero <[email protected]>
…s) (#564) Signed-off-by: Jose Luis Rivero <[email protected]>
…s) (#564) Signed-off-by: Jose Luis Rivero <[email protected]>
…s) (#564) Signed-off-by: Jose Luis Rivero <[email protected]>
…s) (#564) Signed-off-by: Jose Luis Rivero <[email protected]>
The PR solves a critical problem: when building a new component (new shared object) the visibility keyword can not be the same than a library it links to. This translated means: our systems and components can not use
IGNITION_GAZEBO_VISIBLE
but the custom name created by ign-cmake in the form ofIGNITION_GAZEBO_${COMPONENT_NAME}_VISIBLE
. Otherwise there is a nice list of fanky linker errors all around.