BinomialLLC / basis_universal

Basis Universal GPU Texture Codec
Apache License 2.0
2.72k stars 267 forks source link

Incorrect return code when invoked without arguments? #291

Open rumpled opened 2 years ago

rumpled commented 2 years ago

Full disclaimer, I'm a devops guy just trying to support a new project so I had zero experience with basis prior to this.

I was trying to validate that I had the tool correctly installed by simply invoking the basisu command but the output that I got was:

npm i -g basisu fs-extra
basisu
err: Error: Command failed: /usr/local/lib/node_modules/basisu/bin/linux/x64/basisu

You can replicate the error with:

docker run                                  \
  --platform=linux/amd64 -it --rm           \
  --env NPM_CONFIG_UPDATE_NOTIFIER=false    \
  node:16-bullseye-slim                     \
  npx --yes -p fs-extra -p basisu           \
  basisu

I finally tracked down that basisu is returning EXIT_FAILURE after printing the usage message.

https://github.com/BinomialLLC/basis_universal/blob/44e1bcd3e145d18eb29d2915581ad4c61bb65cc7/basisu_tool.cpp#L4384-L4388

This wouldn't ordinarily be an issue, plenty of other linux commands return EXIT_FAILURE when invoked without arguments, but...

When installed via npm, the wrapper invocation script basisu.js invokes the basisu runtime via child_process.execFile and that considers any non-zero return code to be a failure which ends up swallowing the output and leaves the user baffled.

https://github.com/HacksawStudios/basisu/blob/master/bin/basisu.js#L35-L40

It seems to me that printing usage should return a non-error code since the behavior is not an abnormal codepath but that is 100% subjective and there are plenty of examples either way.

Is it a reasonable request to change line 4387 to EXIT_SUCCESS?

richgel999 commented 2 years ago

It seems to me that printing usage should return a non-error code since the behavior is not an abnormal codepath but that is 100% subjective and there are plenty of examples either way.

I don't really know - it's a good point and I've wondered it myself. I'll change this - shouldn't be an issue. Thanks!