gianni-rosato / rav1ator-cli

An easy-to-use CLI utility for working with Av1an, written in Bash.
BSD 3-Clause "New" or "Revised" License
17 stars 2 forks source link

Feedback #1

Open murlakatamenka opened 9 months ago

murlakatamenka commented 9 months ago

As of https://github.com/gianni-rosato/rav1ator-cli/commit/f08513bac43dd16ca353903b740e0cba7df18622

  1. Don't use pacman/<aur helper> -Sy

https://github.com/gianni-rosato/rav1ator-cli/blob/f08513bac43dd16ca353903b740e0cba7df18622/README.md?plain=1#L56

  1. What is rust doing in dependencies?

https://github.com/gianni-rosato/rav1ator-cli/blob/f08513bac43dd16ca353903b740e0cba7df18622/rav1ator-cli#L55

You download pre-compiled binaries and don't build anything like rav1e from source anyway. Rust binaries are statically linked and either require gcc-libs or nothing at all (if built with musl), unless something is explicitly specified.

  1. Consider using CachyOS repos for your pre-compiled dependencies.

https://github.com/gianni-rosato/rav1ator-cli/blob/f08513bac43dd16ca353903b740e0cba7df18622/rav1ator-cli#L23-L28

Wow, this is absolutely horrible. It's as good as me saying "hey, here is very fast rav1e I've compiled, download it from http://trusted.url/rav1e-supafast", i.e. downloading and running random binaries from the Internet.

Sorry, but this is not your concern, there are package managers to manage dependencies. You can only ask the user to have specific binaries bin1..binN in PATH.

I understand the appeal of having faster encoders, such as compiled with -march=native -O3 -flto. But again, that's user's concern, it is in their interests.

If you still want to play this game, then at least consider using a proper sourcefor them, such as CachyOS repos. All you care is those faster x86_64-v3/4 -O3 -flto binaries that use AVX etc, and that's exactly what CachyOS provides. After all it's a whole Linux distro with own repos and signed packages, not some chat attachments :)

https://wiki.cachyos.org/cachyos_repositories/why_cachyos_repo/

As a bonus you'll also get x86_64-v4 binaries for those newer CPUs like Zen 4.

Here are the packages for rav1e, for example:

  1. Rewrite-it-in-Rust (it's a joke about Rust)

Well, not a big fan of shell scripts that don't fit on a single screen. Google even has its own recommendation on the topic:

If you are writing a script that is more than 100 lines long, or that uses non-straightforward control flow logic, you should rewrite it in a more structured language now. Bear in mind that scripts grow. Rewrite your script early to avoid a more time-consuming rewrite at a later date.

I see that you don't even use shellcheck to not fall into common bash pitfalls, that's a bummer.

Something like Rust, Go, C/C++ or Zig (that you learn) can be viable choices. gum that you use in written in Go, and it can also be used as a lib iirc. There is always Python too.

  1. Add PKGBUILD

You refer to Arch a few times, so maybe add it to the repo? I find PKGBUILDs really straightforward and easy to read, they are basically small bash scripts, right? They have all those make/runtime dependencies and which files should be where for the whole package to work properly. Here is one for something you're familiar with:

https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=aviator-git

Just 37 lines, so easy.

Also PKGBUILD kinda solves point 3 about dependencies.


That's it for now, hope the feedback was helpful 😊

gianni-rosato commented 9 months ago

Hello, thank you for your feedback! I want to address these issues one at a time as it seems you've put a considerable amount of effort into voicing your recommendations for my project thoughtfully. Let me break it down;

  1. I'll fix this right away. I wasn't aware of this, thank you for bringing it to my attention.

  2. I'm not sure why I thought Rust was necessary, I will keep it out of the recommended dependencies.

  3. I'll consider putting a disclaimer, but I'm not going to stop shipping precompiled binaries.

I am entirely aware of the fact that the majority of seasoned Linux users are vehemently opposed to downloading and executing unknown binaries on their systems. It is not something I am fond of doing myself, and I build things from source or use a trusted package manager when possible.

However, as I've interacted more with members of the encoding community, I've realized that for beginners these kinds of considerations are difficult to honor all the time. Nearly every Windows user I encounter is happy to download an unknown .exe in order to avoid the tedium of compiling your own software, and I understand this is kind of just the "Windows way," so to speak. I'm assuming you don't use the AUR, which discloses "AUR packages are user produced content. Any use of the provided files is at your own risk."

A slightly faster binary for an encoder is a really big deal. A 1-2% performance improvement makes a considerable difference for encodes that can take hours or even days. Considering I am personally connected to many in the community who use my tools, I understand providing trustworthy binaries is a necessity.

The CachyOS repos will not help me, as aom-av1-lavish and BlueSwordM's SVT-AV1 fork are community forks of aomenc & SVT-AV1 that maintained by one individual each. For the time being, while this tool is still a small, the opportunity cost of not providing pre-compiled binaries is too high at the moment. In the future, I'll consider other avenues.

  1. I'm definitely going to seriously consider re-writing the script in Zig, but for the time being, I believe it isn't hurting for it to be in Bash.

Based on the Google guidelines you provided, they are as follows:

I can break this down into the following:

rAV1ator CLI only fails to meet one of these criteria; if you look at the codebase, everything is relatively simple, and the entire utility is just an Av1an frontend with some helper features.

Now, this isn't to say your request isn't valid; it is, and I am planning a re-write when I have more free time. However, as of right now, I don't think a re-write is an emergency as much as it could be made out to be one.

  1. For a lot of the reasons you've mentioned here, I don't consider rAV1ator CLI complete enough to try to publish it to the repos yet. I would not be comfortable pushing a tool to the Arch repos that performs binary installation outside of the package manager and is just a Bash script.

Thank you for the suggestions, and I hope my response is satisfactory! TL;DR, I'm going to address a good number of them in the future, but I think that honestly may materialize in the form of an entirely new utility that deprecates this one. Thanks again!