Update install.sh

[cyqsimon]

I think this is good enough, although I cannot fully test it until there's a new release using the updated builder.py.

Don't rush with the release though, I want to merge #214 first (which also requires me to change 2 or 3 lines in install.sh).

[rooklift]

Hmm - is it necessary to make such drastic changes? Various people spent some time making it as good as possible last time, and also, since I barely know Linux, I'm nervous about changing it, especially when strings like "sudo rm -rf" pop up...

[cyqsimon]

Hmm - is it necessary to make such drastic changes?

Well, kind of. I made the changes I made because I want the script to be as maintainable as possible going forward, not necessarily taking into account the amount I'm changing.

---

Various people spent some time making it as good as possible last time

I really can't agree that it's "as good as possible", no disrespect to the people who worked on this. There were several things that could be improved that I saw:

1. Not using standard Unix/Linux tools where appropriate.
   • line 5-8: the standard way to determine if a binary is present is which.
   • line 16: the standard way of obtaining a temporary directory is mktemp. Using /tmp directly has many (though mostly rare) pitfalls.
2. Mixed wget and curl usage. This requires both binaries to be present, which is an unnecessary point of failure. Also unlike curl which was checked at the start of the script, wget was never checked before being used.
3. Very sketchy sudo unzip call on line 32. You shouldn't unzip directly into a system directory, for a host of safety reasons.
   • This also does not check/warn/handle overwrites. So in case you have already installed Nibbler from another source (e.g. from AUR), or have an older version installed, this is essentially UB.
4. Asking to install .desktop file is simply unnecessary. Since Nibbler is a desktop GUI application, it's almost a given that the user would want to launch it graphically.
   • Just to clarify, this is not a "desktop shortcut" (although it can be used as such), rather Linux's standard way of registering the application with the Desktop Environment, so that it can be launched graphically from the launcher.
5. General lack of comments. Although the script is rather straightforward, I think we can all agree that more comments is never bad.
6. Mixed code styles - clearly the code was written and then patched over by several authors with very different code styles. I won't say any of them is superior or inferior, but suffice to say it's not good for maintainability.

Again, it's my principle to make code that pass through my hands as high quality as I can. And in this case, it does indeed involve lots of changes.

---

I'm nervous about changing it, especially when strings like "sudo rm -rf" pop up...

That's okay. Any sudo usage is scary, as it should be. Although I'm pretty confident about the quality of my code, I'm happy to have someone else review it, if that floats your boat.

That being said, I would argue that the sudo calls I'm using are actually much safer than the current ones, since the inputs are all hard-coded instead of dependent on the contents of a zip file from the internet.

[cyqsimon]

${INSTALL_DIR} ${BIN_SYMLINK_PATH} ${DESKTOP_ENTRY_PATH} and ${ICON_PATH} are all statically defined. It's just deleting several files at statically-known locations, so there's really nothing unsafe here.

[cyqsimon]

This sudo unzip -d /opt/ on the other hand, I really do not like at all.

[cyqsimon]

This unzips into a tempdir

[cyqsimon]

... and only install statically-known files into system directories.

[rooklift]

I had some people look at it and they seem happy enough, so go ahead.

Uh, are you wanting 2.3.9 to exist before you complete this?

[rooklift]

@cyqsimon - also if you're ready I guess I'll merge the reorganise branch into master?

[cyqsimon]

@cyqsimon - also if you're ready I guess I'll merge the reorganise branch into master?

Sure. I'll notify downstream on AUR to update their build script when you're done.

[rooklift]

I guess.

[rooklift]

@cyqsimon - so everything is done and 2.3.9 now exists.

Uh, I guess the one-liner to install Nibbler is:

bash -c "$(wget -O- https://raw.githubusercontent.com/rooklift/nibbler/master/files/scripts/install.sh)"

Right?

[cyqsimon]

@rooklift I like this more:

curl -L https://raw.githubusercontent.com/rooklift/nibbler/master/files/scripts/install.sh | bash

It's cleaner, and is consistent with the script in that it doesn't require wget.

[rooklift]

OK.