neofetch: remove optional dependencies

[yochem]

First time contributor here, please let me know if I did something wrong :)

The dependencies are optional. Optional dependencies should not be
listed in homebrew.

Source:

• image: Remove imagemagick dependency and draw images
  directly.

• system_profiler is used as fallback (link to code):
  https://github.com/dylanaraps/neofetch/blob/a980dc8e8be54e57a1c64518fb68c166b84cbd2c/neofetch#L3007-L3014

• Have you followed the guidelines for contributing?

• Have you ensured that your commits follow the commit style guide?

• Have you checked that there aren't other open pull requests for the same formula update/change?

• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?

• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?

• Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

---

[SMillerDev]

The dependencies are optional. Optional dependencies should not be listed in homebrew.

This is true, unless they provide benefits to the user.

https://github.com/dylanaraps/neofetch/wiki/Dependencies suggests that both these are required though.

[yochem]

Thank you for your comment and the link you provided! The following is stated at the top of the link you commented:

NOTE: Neofetch only requires BASH, the rest of the dependencies are entirely optional and only enable additional features and functionality.

I saw that homebrew can mark dependencies as optional, would this be an idea?

I want to prevent installing these depencies just because of an optional feature:

$ brew deps neofetch
aom
docbook
docbook-xsl
fontconfig
freetype
gdbm
gettext
ghostscript
glib
gnu-getopt
imagemagick
imath
jbig2dec
jpeg
libde265
libffi
libheif
libidn
liblqr
libomp
libpng
libtiff
libtool
little-cms2
m4
mpdecimal
openexr
openjpeg
[email protected]
pcre
[email protected]
readline
screenresolution
shared-mime-info
sqlite
webp
x265
xmlto
xz

It seems quite heavy for only adding in support for showing images, especially because this is not the default behaviour. (it is showing ascii art, not images).

Let me know what you think.

[SMillerDev]

I saw that homebrew can mark dependencies as optional, would this be an idea?

This is not supported in homebrew-core since it breaks automated testing for formulae.

I want to prevent installing these depencies just because of an optional feature

Most of these are pretty common dependencies for software. If you really want to get rid of them you can always use brew extract to maintain your own, minimal, version of neofetch. I'll also leave this open for other maintainers to weigh in.

[carlocab]

screenresolution is an ~85K dependency that makes this script run faster. Removing it is more trouble than it's worth, I think.

I'm a bit more sympathetic to the removal of imagemagick, though it would be good to clarify whether it's actually not used at all (as suggested by the linked PR) or is actually like screenresolution, where it is used whenever available (as suggested by the wiki).

[yochem]

Yeah keeping screenresolution seems reasonable. The PR is written by the author, the wiki written by someone else. The output of neofetch with default configuration is the same for both with imagemagick and without:

$ neofetch --config none > with_magick
$ brew uninstall --ignore-dependencies imagemagick
$ neofetch --config none > without_magick
$ diff with_magick without_magick
24c24
<                                  Packages: 169 (brew)
---
>                                  Packages: 168 (brew)
32c32
<                                  Memory: 6131MiB / 8192MiB
---
>                                  Memory: 6121MiB / 8192MiB

The only difference is of course the removed package and a change in memory, not in the ascii art of the OS logo. Let's check the difference in time:

Without imagemagick:

$ time neofetch --config none >/dev/null

________________________________________________________
Executed in  788.24 millis    fish           external
   usr time  401.61 millis  134.00 micros  401.47 millis
   sys time  320.00 millis  741.00 micros  319.25 millis

With imagemagick:

$ time neofetch --config none >/dev/null
________________________________________________________
Executed in  855.71 millis    fish           external
   usr time  413.30 millis  140.00 micros  413.16 millis
   sys time  334.53 millis  680.00 micros  333.85 millis

Not that much difference.

It seems that neofetch can use the convert binary from imagemagick. The link to the code part where it's used is here: https://github.com/dylanaraps/neofetch/blob/master/neofetch#L4372-L4414.

It's in make_thumbnail() which gets called only in image_backend() If the backend is not off or ascii. In the default config (which is a giant block of text in the neofetch file) the image_backend is set to ascii: https://github.com/dylanaraps/neofetch/blob/master/neofetch#L757.

[kidonng]

I'm in favor of removing the imagemagick dependency. Actually I reverted 18c76cc in my own tap.

it would be good to clarify whether it's actually not used at all (as suggested by the linked PR) or is actually like screenresolution, where it is used whenever available (as suggested by the wiki).

It's not used at all with the default config, and I've never seen anyone posting neofetch screenshots enabling the image option (well, except the author themself). Plus the feature requires supported terminals and has its own quirks.

To me this is just pulling down lots of dependencies people rarely need.

[carlocab]

I'm fine with removing imagemagick. We should probably keep screenresolution.

Oh, and please rebase to pull in changes from #79442. (With git rebase, please. We don't want merge commits on your PR branch. Apologies if I'm stating the obvious, but experience has shown that this reminder is sometimes useful.)

[yochem]

Sounds good! Thank you for your reminder. I will rebase and and add the dependency on screenresolution back in.

[carlocab]

Great. Wheen you add screenresolution back, please do it by amending your original commit (as opposed to adding a new one).

[yochem]

Hi! I did the following:

$ brew update
$ cd (brew --repository homebrew/core)
$ git checkout neofetch-remove-deps
$ git rebase master
$ brew edit  neofetch
$ git add Formula/neofetch.rb
$ git commit --amend
$ git push [email protected]:yochem/homebrew-core.git neofetch-remove-deps
To github.com:yochem/homebrew-core.git
 ! [rejected]              neofetch-remove-deps -> neofetch-remove-deps (non-fast-forward)
error: failed to push some refs to 'github.com:yochem/homebrew-core.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

How could this happen? I'm pretty sure I'm not behind the remote.

[yochem]

Ah, as per 'Note about fast-forwards in the git push man page:

"There is another common situation where you may encounter non-fast-forward rejection when you try to push, and it is possible even when you are pushing into a repository nobody else pushes into. After you push commit A yourself (in the first picture in this section), replace it with "git commit --amend" to produce commit B, and you try to push it out, because forgot that you have pushed A out already. In such a case, and only if you are certain that nobody in the meantime fetched your earlier commit A (and started building on top of it), you can run "git push --force" to overwrite it. In other words, "git push --force" is a method reserved for a case where you do mean to lose history."

So git push --force would solve this? I'm not doing this confirmation because it feels weird to force push haha.

[carlocab]

git push --force is exactly what you need here. It's what you need when you're rewriting commit history. (Generally frowned upon if others are using your branch, but for PR branches it should be fine. You need it after having done git rebase anyway, as that also rewrites history.)

[yochem]

Ready for review!

[carlocab]

Thanks, @yochem.

[BrewTestBot]

🤖 A scheduled task has triggered a merge.