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

Updated findfile() to search localpath first #1292

Merged
merged 6 commits into from
Jun 26, 2023

Conversation

jasmeet0915
Copy link
Contributor

@jasmeet0915 jasmeet0915 commented Jun 13, 2023

🦟 Bug fix

Fixes #1172

Summary

As the issue above mentions, this tutorial for Garden, is not able to parse the SDF file if it was named model.sdf. It works if the file name is changed. This was happening due to the search order used in the sdf::findFile() function. The used search order was causing the findFile() to find the model.sdf from the install path instead of the one present locally. Since the found model.sdf is actually the SDF spec file for the <model> and it is not a valid SDFormat file, it caused the script to throw a not valid error.
This PR changes the search order used in the findFile() function by searching for the local path (current directory) first and solves the issue.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jun 13, 2023
@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #1292 (5ae2f98) into sdf13 (b363e0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5ae2f98 differs from pull request most recent head 93bd441. Consider uploading reports for the commit 93bd441 to get more accurate results

@@           Coverage Diff           @@
##            sdf13    #1292   +/-   ##
=======================================
  Coverage   87.59%   87.60%           
=======================================
  Files         128      128           
  Lines       16826    16827    +1     
=======================================
+ Hits        14739    14741    +2     
+ Misses       2087     2086    -1     
Impacted Files Coverage Δ
src/SDF.cc 95.85% <100.00%> (+0.61%) ⬆️

src/SDF.cc Outdated Show resolved Hide resolved
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

this makes sense to me

@scpeters
Copy link
Member

this makes sense to me

though we mention the change in the Migration guide

Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
@azeey
Copy link
Collaborator

azeey commented Jun 21, 2023

Can you add a test that creates a temporary model.sdf in the current directory and verifies that sdf::readFile reads the correct file? Also, could you address @scpeters's request to add the change to the Migration guide?

Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
@jasmeet0915
Copy link
Contributor Author

@azeey I have added a test that, as suggested, creates a temporary model.sdf in the current directory and verifies that the correct path is returned by the sdf::findFile(). Also updated the migration guide as @scpeters suggested.

Copy link
Contributor

@quarkytale quarkytale left a comment

Choose a reason for hiding this comment

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

Works as expected!

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Works great! Just have one small comment.

src/SDF_TEST.cc Outdated Show resolved Hide resolved
@azeey azeey enabled auto-merge (squash) June 26, 2023 22:16
@azeey azeey merged commit da1f325 into gazebosim:sdf13 Jun 26, 2023
@scpeters scpeters mentioned this pull request Aug 30, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

model.sdf not parsed
5 participants