gz fuel download returns 0, even when the download failed

[deniz-hofmeister]

Environment

• OS Version: Ubuntu 22.04, binary build
• gz-fuel-tools:
  type: git
  url: https://github.com/gazebosim/gz-fuel-tools
  version: gz-fuel-tools8
  commit: 54491c8

Description

• Expected behavior: return code non-zero
• Actual behavior:error code zero

root@user-desktop:/gazebo# gz fuel download --url https://fuel.gazebosim.org/1.0/openrobotics/models/sydney_regatta
[Err] [FuelClient.cc:677] Failed to download model.
  Server: https://fuel.gazebosim.org
  Route: openrobotics/models/sydney_regatta/tip/sydney_regatta.zip
  REST response code: 504
Download failed.
root@user-desktop:/gazebo# echo $?
0

It would be nice to have a non-zero return code in the terminal, as it would make error handling and catching easier.

Greetings

[M1chaelM]

This bug is particularly bad when working with Docker images. Since resources are not typically downloaded and cached until the first time they are needed, users running gz sim commands from a Docker image have to download everything from fuel every time unless they use gz fuel download to build the cache into the image. However, Docker relies on the error code to determine whether a step in its build process has succeeded. So when gz fuel download returns 0 even though it has failed, the result is that Docker does not detect the error, and bakes the failed download into the image.

If the user is not actively reading the console output they will think they've cached the resource when they haven't. Further, to fix the problem, the user will have to manually add a cache-bust argument to the appropriate spot in the Dockerfile, since Docker has no way of knowing which layer to start from.

[rahul-k-a]

I can make a quick fix for this. Since gz-fuel-tools8 doesnt seem to be in active development, shall I push the changes directly in gz-fueld-tools10 instead?

[rahul-k-a]

Looking into the source code. it seems the only change that I need to make is replacing

if (client.DownloadWorld(id))
    return 0

to

 if (client.DownloadWorld(id).Type() == ResultType::FETCH)
   return 0

Could someone confirm?

[kscottz]

@rahul-k-a That seems legitimate to me. Send over a PR and we can give it a try.

[rahul-k-a]

@kscottz just to confirm (since this is my first time contributing to gazebo), I make a branch off of the main branch, correct? I'm confused as to which branch is currently under active development

[rahul-k-a]

PR is open: #459 . Please let me know if there's anything that I need to change.

[rahul-k-a]

Also, I am not sure if this comes under the purview of this specific ticket, but I noticed that we return -1 on failure in the examples/download.cc. Some operating systems use an unsigned 8-bit integer to store the return values, so returning -1 might cause an underflow and make the shell return 255 when doing echo $?. Would returning 0 for failure and 1 for success be more compatible?