Skip to content
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

Upgrade cpplint and fix new errors #680

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Aug 31, 2021

Summary

Upgrade cpplint to the version found in ign-cmake2. @scpeters and I had discussed using ign-cmake to create a codecheck target, but it looks like we don't directly depend on ign-cmake in the sdf9 branch. So instead, I simply copied cpplint.py from ign-cmake. We might be able to add the codecheck target for sdf10

Test it

sh tools/code_check.sh

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey self-assigned this Aug 31, 2021
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel labels Aug 31, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #680 (6ddc3eb) into sdf9 (e0d77cc) will decrease coverage by 0.00%.
The diff coverage is 90.90%.

❗ Current head 6ddc3eb differs from pull request most recent head f5c8bb3. Consider uploading reports for the commit f5c8bb3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             sdf9     #680      +/-   ##
==========================================
- Coverage   86.83%   86.83%   -0.01%     
==========================================
  Files          62       62              
  Lines        9734     9731       -3     
==========================================
- Hits         8453     8450       -3     
  Misses       1281     1281              
Impacted Files Coverage Δ
include/sdf/Element.hh 100.00% <ø> (ø)
src/Error.cc 100.00% <ø> (ø)
src/FrameSemantics.cc 76.54% <ø> (ø)
src/Joint.cc 92.02% <ø> (ø)
src/Utils.hh 100.00% <ø> (ø)
src/parser.cc 77.63% <ø> (ø)
src/parser_urdf.cc 79.16% <84.61%> (-0.04%) ⬇️
src/Light.cc 88.00% <100.00%> (ø)
src/Link.cc 92.30% <100.00%> (ø)
src/Model.cc 83.54% <100.00%> (ø)
... and 4 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 e0d77cc...f5c8bb3. Read the comment docs.

@azeey azeey requested a review from marcoag August 31, 2021 20:00
sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.6", "1_5.convert");
const std::string CONVERT_DOC_16_17 =
sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.7", "1_6.convert");
static std::string ConvertDoc_15_16()
Copy link
Contributor

Choose a reason for hiding this comment

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

So did cpplint actually catch this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant to mention this in the description, but yes, cpplint caught the global string as a runtime/string error:
https://github.com/azeey/sdformat/blob/f5c8bb383fe13d2417f960afb261d626f1cbd40c/tools/cpplint.py#L5455. This helps some with the GSG violations we talked about, but it only works for strings.

@scpeters
Copy link
Member

scpeters commented Sep 1, 2021

it looks like we don't directly depend on ign-cmake in the sdf9 branch

we depend on ignition-math6, which depends on ignition-cmake2, so I consider it a valid dependency. It would only be used as a build dependency anyway

@azeey
Copy link
Collaborator Author

azeey commented Sep 1, 2021

we depend on ignition-math6, which depends on ignition-cmake2, so I consider it a valid dependency. It would only be used as a build dependency anyway

Okay, I knew about the dependency, but I wasn't sure if it would be possible to build sdformat with just ign-math installed and ign-cmake not installed. If it's okay to add the dependency, I'll do a followup PR that adds the codecheck target.

@azeey azeey merged commit 1ae08ad into gazebosim:sdf9 Sep 1, 2021
@azeey azeey deleted the upgrade_cpplint branch September 1, 2021 16:51
@scpeters
Copy link
Member

scpeters commented Sep 1, 2021

we depend on ignition-math6, which depends on ignition-cmake2, so I consider it a valid dependency. It would only be used as a build dependency anyway

Okay, I knew about the dependency, but I wasn't sure if it would be possible to build sdformat with just ign-math installed and ign-cmake not installed. If it's okay to add the dependency, I'll do a followup PR that adds the codecheck target.

cmake fails to find ignition-math6 without ignition-cmake2. I used brew unlink ignition-cmake2 on my macOS machine and then get the following configuration error with sdf9:

-- Looking for ignition-math6-config.cmake - not found
-- 	Missing: Ignition math (libignition-math6-dev)
...
CMake Error at CMakeLists.txt:240 (message):
  -- BUILD ERRORS: These must be resolved before compiling.


CMake Error at CMakeLists.txt:242 (message):
  -- 	Missing: Ignition math (libignition-math6-dev)


CMake Error at CMakeLists.txt:244 (message):
  -- END BUILD ERRORS



CMake Error at CMakeLists.txt:245 (message):
  Errors encountered in build.  Please see the BUILD ERRORS above.

so I think it's safe to add an explicit dependency

@azeey azeey mentioned this pull request Sep 1, 2021
8 tasks
@azeey
Copy link
Collaborator Author

azeey commented Sep 1, 2021

Sounds good. I've created a follow up PR #682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants