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

Use semantic version and prevent crash if version is missing #151

Merged
merged 2 commits into from
Jan 4, 2021

Conversation

chapulina
Copy link
Contributor

I was getting a crash while downloading a model with a malformed config:

$ ign fuel download -j 4 -v 4 -u "https://fuel.ignitionrobotics.org/1.0/openrobotics/models/Universal Robotics UR10 robot arm"
...
terminate called after throwing an instance of 'std::logic_error'                                                                           
  what():  basic_string::_M_construct null not valid                     
Aborted (core dumped)   

The problem is that the <sdf> element of that model doesn't have a version attribute.

To prevent a crash, I'm assuming a version 0.0.1.

Which begs the question, is that version meant to be semantic? I don't think that has been specified anywhere. But considering that most models have a version in the format X.X, I think semantic versioning would make more sense than doubles. That is, 1.12 is larger than 1.2. So I also updated the logic to use Ignition Math's SemanticVersion class. This adds an explicit dependency on ign-math, but that should be ok, because that was already an indirect dependency through ign-common and ign-msgs.

@chapulina chapulina requested a review from nkoenig as a code owner December 23, 2020 02:56
Signed-off-by: Louise Poubel <[email protected]>
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Dec 23, 2020
@codecov
Copy link

codecov bot commented Dec 23, 2020

Codecov Report

Merging #151 (bb85340) into ign-fuel-tools4 (21e8b0f) will decrease coverage by 0.06%.
The diff coverage is 62.50%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           ign-fuel-tools4     #151      +/-   ##
===================================================
- Coverage            78.55%   78.48%   -0.07%     
===================================================
  Files                   19       19              
  Lines                 2583     2589       +6     
===================================================
+ Hits                  2029     2032       +3     
- Misses                 554      557       +3     
Impacted Files Coverage Δ
src/LocalCache.cc 79.58% <62.50%> (-0.54%) ⬇️

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 21e8b0f...bb85340. Read the comment docs.

@chapulina chapulina merged commit 99e0bcb into ign-fuel-tools4 Jan 4, 2021
@chapulina chapulina deleted the chapulina/semantic branch January 4, 2021 23:37
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants