Skip to content

Resolve updated codecheck issues #443

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 10, 2020
Merged

Resolve updated codecheck issues #443

merged 2 commits into from
Nov 10, 2020

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Nov 4, 2020

This PR is meant to resolve new codecheck issues introduced by gazebosim/gz-cmake#117.

From my understanding, the plan is to forward port the fixes here to all other ignition versions once gazebosim/gz-cmake#117 is merged.

Signed-off-by: Ashton Larkin [email protected]

@adlarkin
resolved codecheck issues
06535e1 
Signed-off-by: Ashton Larkin <[email protected]>
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Nov 4, 2020
@adlarkin adlarkin mentioned this pull request Nov 4, 2020
Merged
chapulina
chapulina suggested changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces lots of failures to the local ./tools/code_check.sh:

  ./src/gui/PathManager.cc:23:  Found C system header after C++ system header. Should be: PathManager.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/view_angle/ViewAngle.cc:23:  Found C system header after C++ system header. Should be: ViewAngle.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/view_angle/ViewAngle.cc:24:  Found C system header after C++ system header. Should be: ViewAngle.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/transform_control/TransformControl.cc:23:  Found C system header after C++ system header. Should be: TransformControl.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/transform_control/TransformControl.cc:24:  Found C system header after C++ system header. Should be: TransformControl.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/shapes/Shapes.cc:24:  Found C system header after C++ system header. Should be: Shapes.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/shapes/Shapes.cc:25:  Found C system header after C++ system header. Should be: Shapes.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/video_recorder/VideoRecorder.cc:23:  Found C system header after C++ system header. Should be: VideoRecorder.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/video_recorder/VideoRecorder.cc:24:  Found C system header after C++ system header. Should be: VideoRecorder.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/playback_scrubber/PlaybackScrubber.cc:27:  Found C system header after C++ system header. Should be: PlaybackScrubber.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/playback_scrubber/PlaybackScrubber.cc:28:  Found C system header after C++ system header. Should be: PlaybackScrubber.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/modules/EntityContextMenu.cc:23:  Found C system header after C++ system header. Should be: EntityContextMenu.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/modules/EntityContextMenu.cc:24:  Found C system header after C++ system header. Should be: EntityContextMenu.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/modules/EntityContextMenu.cc:25:  Found C system header after C++ system header. Should be: EntityContextMenu.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/resource_spawner/ResourceSpawner.cc:26:  Found C system header after C++ system header. Should be: ResourceSpawner.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/gui/plugins/resource_spawner/ResourceSpawner.cc:27:  Found C system header after C++ system header. Should be: ResourceSpawner.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/triggered_publisher/TriggeredPublisher.cc:23:  Found C system header after C++ system header. Should be: TriggeredPublisher.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/triggered_publisher/TriggeredPublisher.cc:24:  Found C system header after C++ system header. Should be: TriggeredPublisher.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/physics/Physics.cc:27:  Found C system header after C++ system header. Should be: Physics.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/physics/Physics.cc:28:  Found C system header after C++ system header. Should be: Physics.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/physics/Physics.cc:29:  Found C system header after C++ system header. Should be: Physics.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/joint_position_controller/JointPositionController.cc:22:  Found C system header after C++ system header. Should be: JointPositionController.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/altimeter/Altimeter.cc:24:  Found C system header after C++ system header. Should be: Altimeter.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/log_video_recorder/LogVideoRecorder.cc:24:  Found C system header after C++ system header. Should be: LogVideoRecorder.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/log_video_recorder/LogVideoRecorder.cc:25:  Found C system header after C++ system header. Should be: LogVideoRecorder.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/joint_controller/JointController.cc:22:  Found C system header after C++ system header. Should be: JointController.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/battery_plugin/LinearBatteryPlugin.cc:27:  Found C system header after C++ system header. Should be: LinearBatteryPlugin.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/battery_plugin/LinearBatteryPlugin.cc:28:  Found C system header after C++ system header. Should be: LinearBatteryPlugin.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/logical_camera/LogicalCamera.cc:25:  Found C system header after C++ system header. Should be: LogicalCamera.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/user_commands/UserCommands.cc:24:  Found C system header after C++ system header. Should be: UserCommands.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/user_commands/UserCommands.cc:25:  Found C system header after C++ system header. Should be: UserCommands.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/user_commands/UserCommands.cc:26:  Found C system header after C++ system header. Should be: UserCommands.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/user_commands/UserCommands.cc:27:  Found C system header after C++ system header. Should be: UserCommands.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/wind_effects/WindEffects.cc:23:  Found C system header after C++ system header. Should be: WindEffects.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/wind_effects/WindEffects.cc:24:  Found C system header after C++ system header. Should be: WindEffects.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/wind_effects/WindEffects.cc:25:  Found C system header after C++ system header. Should be: WindEffects.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/pose_publisher/PosePublisher.cc:27:  Found C system header after C++ system header. Should be: PosePublisher.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/contact/Contact.cc:25:  Found C system header after C++ system header. Should be: Contact.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/contact/Contact.cc:26:  Found C system header after C++ system header. Should be: Contact.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/diff_drive/DiffDrive.cc:25:  Found C system header after C++ system header. Should be: DiffDrive.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/scene_broadcaster/SceneBroadcaster.cc:24:  Found C system header after C++ system header. Should be: SceneBroadcaster.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/log/LogPlayback.cc:24:  Found C system header after C++ system header. Should be: LogPlayback.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/log/LogPlayback.cc:25:  Found C system header after C++ system header. Should be: LogPlayback.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/joint_state_publisher/JointStatePublisher.cc:23:  Found C system header after C++ system header. Should be: JointStatePublisher.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/breadcrumbs/Breadcrumbs.cc:25:  Found C system header after C++ system header. Should be: Breadcrumbs.h, c system, c++ system, other.  [build/include_order] [4]
  ./src/systems/air_pressure/AirPressure.cc:24:  Found C system header after C++ system header. Should be: AirPressure.h, c system, c++ system, other.  [build/include_order] [4]

@adlarkin
fixing local code checker
1e400bd 
Signed-off-by: Ashton Larkin <[email protected]>
@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 4, 2020

This introduces lots of failures to the local ./tools/code_check.sh

Sorry about that; I forgot that google protobuf headers are considered as standard c headers. The local check and make codecheck should both work now after 1e400bd.

@codecov
Copy link

codecov bot commented Nov 4, 2020 •
edited
Loading

Codecov Report

Merging #443 (1e400bd) into ign-gazebo2 (8856e72) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo2     #443   +/-   ##
============================================
  Coverage        78.00%   78.00%           
============================================
  Files              187      187           
  Lines            10208    10208           
============================================
  Hits              7963     7963           
  Misses            2245     2245
Impacted Files Coverage Δ
src/LevelManager.cc 90.71% <ø> (ø)
src/SdfGenerator.cc 95.68% <ø> (ø)
src/SimulationRunner.cc 96.39% <ø> (ø)
src/gui/PathManager.cc 88.88% <ø> (ø)
src/gui/plugins/entity_tree/EntityTree.cc 10.52% <ø> (ø)
src/gui/plugins/modules/EntityContextMenu.cc 19.14% <ø> (ø)
src/gui/plugins/shapes/Shapes.cc 31.25% <ø> (ø)
.../gui/plugins/transform_control/TransformControl.cc 33.33% <ø> (ø)
src/ign.cc 68.91% <ø> (ø)
src/network/NetworkManagerPrimary.cc 78.89% <ø> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8856e72...1e400bd. Read the comment docs.

@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 6, 2020 •
edited
Loading

It looks like ubuntu ci failed. When I took a look at jenkins, it says the following:

apt-get was unable to locate some packages needed for the build. It might be a problem with setup the repositories or for exotic arches the packages might now exists at all.

I'm also seeing test failures on homebrew ci. It looks like one of the tests might be an issue that already exists (PeerTracker has failed for the past 3 builds), but another one of the tests might be new (INTEGRATION_level_manager).

Could it be possible that fixing the headers broke the tests somehow, or are these false negatives? INTEGRATION_level_manager passes all tests when I run it locally, but my local OS is Ubuntu Bionic, so perhaps there's something that's different on Mac.

@chapulina
Copy link
Contributor

ubuntu ci failed

I think this is unrelated to this PR and should be fixed by gazebo-tooling/release-tools#339. Note how it's trying to install dependencies that aren't needed by ign-gazebo2 and are only available from nightlies, like libignition-rendering5-dev.

@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 6, 2020

I see - can this be merged then?

@adlarkin adlarkin requested a review from chapulina November 9, 2020 19:19
@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 9, 2020

@chapulina it looks like merging is blocked until your request for changes is lifted, so I re-requested a review from you.

@chapulina
Copy link
Contributor

@osrf-jenkins run tests one more time please

@chapulina chapulina merged commit 99178d7 into ign-gazebo2 Nov 10, 2020
@chapulina chapulina deleted the adlarkin/cpplint_fixes branch November 10, 2020 21:55
@chapulina chapulina mentioned this pull request Nov 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@chapulina chapulina chapulina approved these changes

@scpeters scpeters Awaiting requested review from scpeters

@azeey azeey Awaiting requested review from azeey azeey is a code owner

@iche033 iche033 Awaiting requested review from iche033 iche033 is a code owner

Assignees
No one assigned
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adlarkin @chapulina @mjcarroll