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.

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

Fix wget in maritime tutorials #2330

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Fix wget in maritime tutorials #2330

merged 2 commits into from
Mar 12, 2024

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Mar 6, 2024

🦟 Bug fix

Fixes #2329

Summary

There were some problems with the wget commands. This patch should fix it.

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.

@caguero
Fix wget in maritime tutorials.
5ccb0c4 
Signed-off-by: Carlos Agüero <[email protected]>
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Mar 6, 2024
@caguero caguero mentioned this pull request Mar 6, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Mar 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.97%. Comparing base (2ca64d0) to head (2d27c3f).

Additional details and impacted files 
@@             Coverage Diff             @@
##           gz-sim8    #2330      +/-   ##
===========================================
+ Coverage    65.95%   65.97%   +0.01%     
===========================================
  Files          327      327              
  Lines        31347    31347              
===========================================
+ Hits         20676    20681       +5     
+ Misses       10671    10666       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arjo129
arjo129 reviewed Mar 7, 2024
View reviewed changes
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Changes work for me now. However, it seems like we are also commiting a zip file and then removing the old files? Shouldnt we download just the zip or use a git commit hash in the url?

@caguero
Copy link
Contributor Author

caguero commented Mar 8, 2024

Changes work for me now. However, it seems like we are also commiting a zip file and then removing the old files? Shouldnt we download just the zip or use a git commit hash in the url?

@arjo129 , could you clarify that?

mabelzhang
mabelzhang reviewed Mar 11, 2024
View reviewed changes
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

I think Arjo is talking about these lines marked below.

It seems maybe the wget by directory was not working, so the new changes are wgetting the zip file and the model files individually.

tutorials/surface_vehicles.md Outdated
@@ -28,8 +28,8 @@ To compile all the custom libraries in the right order `colcon` is recommended.
The `colcon` tool is available on all platforms using `pip3`.

```bash
mkdir ~/gazebo_maritime_ws
wget https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/surface_vehicles/gazebo_maritime_ws -o ~/gazebo_maritime_ws -r
wget https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/gz_maritime_ws.zip -O ~/gz_maritime_ws.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment #2330 (review) is talking about this line.

But it appears the file added is tutorials/files/surface_vehicles/gz_maritime_ws.zip (under surface_vehicles), whereas this line is referring to a zip file in underwater_vehicles? Is that a typo?

The individual files removed are in surface_vehicles here: https://github.com/gazebosim/gz-sim/tree/gz-sim8/tutorials/files/surface_vehicles
There's no zip file in underwater_vehicles right now, nor in this PR: https://github.com/gazebosim/gz-sim/tree/gz-sim8/tutorials/files/underwater_vehicles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that was a typo, thanks! Fixed in 2d27c3f.

tutorials/underwater_vehicles.md
Comment on lines +41 to +49
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/model.config -O ~/gazebo_maritime/models/my_lrauv/model.config
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/model.sdf -O ~/gazebo_maritime/models/my_lrauv/model.sdf
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/materials/textures/Tethys_Albedo.png -O ~/gazebo_maritime/models/my_lrauv/materials/textures/Tethys_Albedo.png
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/materials/textures/Tethys_Metalness.png -O ~/gazebo_maritime/models/my_lrauv/materials/textures/Tethys_Metalness.png
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/materials/textures/Tethys_Normal.png -O ~/gazebo_maritime/models/my_lrauv/materials/textures/Tethys_Normal.png
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/materials/textures/Tethys_Roughness.png -O ~/gazebo_maritime/models/my_lrauv/materials/textures/Tethys_Roughness.png
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/meshes/base.dae -O ~/gazebo_maritime/models/my_lrauv/meshes/base.dae
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/meshes/propeller.dae -O ~/gazebo_maritime/models/my_lrauv/meshes/propeller.dae
wget -r https://raw.githubusercontent.com/gazebosim/gz-sim/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv/meshes/tethys.dae -O ~/gazebo_maritime/models/my_lrauv/meshes/tethys.dae
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment #2330 (review) is talking about these lines

These individual files are in underwater_vehicles here: https://github.com/gazebosim/gz-sim/tree/gz-sim8/tutorials/files/underwater_vehicles/my_lrauv
These seem to be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you see anything suspicious there.

@arjo129
Copy link
Contributor

arjo129 commented Mar 11, 2024

Yeah mabel is right. I think I got confused by the deletion of some of the files.

@caguero
Fix link.
2d27c3f 
Signed-off-by: Carlos Agüero <[email protected]>
arjo129
arjo129 approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Works for me.

@caguero caguero merged commit 7143110 into gz-sim8 Mar 12, 2024
9 of 10 checks passed
@caguero caguero deleted the caguero/fix_wget branch March 12, 2024 08:42
GauravKumar9920 pushed a commit to GauravKumar9920/gz-sim that referenced this pull request Mar 30, 2024
@caguero @GauravKumar9920
Fix wget in maritime tutorials (gazebosim#2330)
8c63d88 
* Fix wget in maritime tutorials.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mabelzhang mabelzhang mabelzhang left review comments

@ahcorde ahcorde ahcorde approved these changes

@arjo129 arjo129 arjo129 approved these changes

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

Assignees
No one assigned
Labels
🎵 harmonic Gazebo Harmonic
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] surface_vehicles.md workspace wget command not working
4 participants
@caguero @arjo129 @ahcorde @mabelzhang