search

huggingface / huggingface_hub

The official Python client for the Huggingface Hub.
https://huggingface.co/docs/huggingface_hub
Apache License 2.0
1.82k stars 470 forks source link

Setting execution bit on scripts with shebang #2345

Open jpodivin opened 2 weeks ago

jpodivin commented 2 weeks ago

The best practice calls for executable scripts, such as those with shebang, to have their execution bits set. Among other things, this helps to immediately highlight them as scripts in the file system.

Wauplin commented 1 week ago

Hi @jpodivin could you provide me some references about the claims "it's best practice to set permission for scripts with shebang"? I'm a bit chilly to change those permissions to be honest. The modified files are never executed on there own. Scrips in ./utils are executed using make style / make quality commands and the CLI one is used by huggingface-cli.

I would be fine merging a PR that removes the shebang from the files you've referenced though. I don't think they have any purpose to be used on their own.

jpodivin commented 2 days ago

The general approach, mostly for ease of use, is to set execution bits on files that are meant to be executed, and provide shebang in scripts so that the right interpreter can be chosen for them. When the bit is missing, it usually indicates that the permissions were not committed properly (that happens way too often), or that filesystem without permissions was used when the files were added (slightly less often). Either way there is little reason to believe it's on purpose and not a mistake, because shebang in non-executable is pretty much useless (you can only run the file if you provide interpreter manually).

I don't really know when it got started, but it's embedded all the way to pre-commit If they are never executed directly they don't need shebang or +x.

The permission bit does have a small, secondary, benefit, in that it simplifies finding executable files.

If these were never meant to be directly executable, and from the looks if it they were not. Removing shebangs is preferrable alternative to make sure it's clear.

Wauplin commented 2 days ago

Thanks for the more detailed explanation @jpodivin, that makes it much clearer to me. I can confirm that those 4 files are never meant to be executed on their own so the correct fix would be to remove the shebang on those. Would you like to update the PR in that sense? Thanks!