3
\$\begingroup\$

The script runs fine, and it seems to be working correctly. I'm looking for some criticism of the structure of the code, errors, bad practices, beginner's pitfalls, and bad code in general. I'm looking forward to improving my understanding of shell scripting.

#!/bin/bash #Setting variables according to user preferences. echo "What is it going to be your hostname?" read -r yourname echo "Is this a laptop, (1)YES, (0)NO?" read -r laptopyn echo "the CPU is INTEL(1) or AMD(0)" read -r cpu echo "If you would like to reenter the information type (1)YES; (2)NO" read -r again #Loop back to first echo if again -eq 1 #if [ $again -eq 1 ] #Setting hostname. echo ">>>>Setting you up Champ" echo "$yourname are you sure?" sleep 2 hostnamectl set-hostname "$yourname" echo "Your hostname is set to:" hostname sleep 2 #Updating OS & disable NtwrkMngr-wait echo ">>>>Starting updates" sudo dnf -y upgrade sudo dnf -y upgrade --refresh sudo dnf -y update sudo dnf -y groupupdate core sudo fwupdmgr refresh --force sudo fwupdmgr update --force sudo systemctl disable NetworkManager-wait-online.service echo ">>>>You are up to date 1/10 " sleep 1 #Downloading, Installing & Cp already customized conf file to correct path echo ">>>>Installing Kitty terminal" sudo dnf install wget -y sudo dnf -y install vim sudo dnf -y install kitty #conf file missing the correct font:.Iosevak sudo cp -r ~/basicfedorasetup/kitty.conf ~/.config/kitty/ echo ">>>>Kitty INSTALLED and setup 2/10" #Enabling rpm-fusion & flatpak Repositories echo ">>>>Sarting with the repo's" sudo dnf install -y https://download1.rpmfusion.org/nonfree/fedora/rpmfusion-nonfree-release-$(rpm -E %fedora).noarch.rpm echo ">>>>rpmfusion INSTALLED 3/10" sudo dnf install -y flatpak flatpak remote-add --if-not-exists flathub https://dl.flathub.org/repo/flathub.flatpakrepo echo ">>>>flatpack INSTALLED 4/10" sleep 2 #Installing essential software "for my use case & preferences" echo ">>>>Installing essentials" sudo dnf install -y 'google-roboto*' 'mozilla-fira*' fira-code-fonts flatpak install flathub -y com.mattjakeman.ExtensionManager sudo dnf install -y unzip p7zip p7zip-plugins unrar sudo dnf install -y gnome-tweaks sudo dnf install -y gimp sudo dnf install -y gimp-devel sudo dnf install -y nautilus-python echo ">>>>google-roboto mozilla-firra,flathpak,unzip,gnome-tweaks,gimp,nautilus-python; INSTALLED 5/10" #Installing some of my most ,non native, used commands echo ">>>>Installing Commands" sudo dnf install -y tmux sudo dnf install -y iostat sudo dnf install -y htop sudo dnf install -y copr-cli rpm -q cronie rpm -q cronie-anacron sudo dnf install -y cronie sudo dnf install -y cronie-anacron echo ">>>>tmux,iostat,htop,copr-cli,cronie,cronie-anacron; INSTALLED 6/10" #Installing most of the software with visual interface that i'm using echo ">>>>Installing Basic Software" flatpak install -y flathub com.google.Chrome flatpak install -y flathub com.discordapp.Discord flatpak install -y flathub com.spotify.Client sudo dnf copr enable -y jerrycasiano/FontManager sudo dnf install -y font-manager echo ">>>Chrome,Discord,Spotify,Font-Manager; INSTALLED 7/10" sleep 2 #If marked as 1(YES), then installing battery optmazation for laptops if [ "$laptopyn" -eq 1 ] then echo ">>>>Starting Laptop Optimazation" sudo dnf -y install tlp tlp-rdw sudo systemctl mask power-profiles-daemon sudo dnf install -y powertop sudo powertop --auto-tune echo ">>>>Laptop Optimazation DONE 8/10" else echo ">>>>This isn't a laptop" echo ">>>>OK 8/10" sleep 2 fi #Multimedia drivers echo ">>>>Sound&Video" sudo dnf update -y @sound-and-video sudo dnf install -y Multimedia sudo dnf install -y ffmpeg ffmpeg-libs libva libva-utils --allowerasing echo ">>>>Sound&Video drivers DONE 9/10" #CPU brand specific drivers. if [ "$cpu" -eq 1 ] then echo ">>>>Installing Intel drivers" sudo dnf -y swap libva-intel-media-driver intel-media-driver --allowerasing else echo ">>>>Installing AMD drivers" sudo dnf -y swap mesa-va-drivers mesa-va-drivers-freeworld sudo dnf -y swap mesa-vdpau-drivers mesa-vdpau-drivers-freeworld fi echo "CPU drivers DONE 10/10" #Done, maybe echo ">>>>ALL SET AND READY" sleep 2 echo ">>>>>Your machine will be restarted in 5sec for all changes to take place" sleep 5 sudo reboot 
\$\endgroup\$
0

    4 Answers 4

    3
    \$\begingroup\$

    Documentation

    Add comments to the top of the file describing the purpose of the code, the expected input and any possible important side effects.

    # Automate post install process on Fedora # This is what I mean by post install... # Some important side effects are... 

    Comments

    The comment phrase Setting variables is not necessary since it is obvious what the read command does. This is a simpler comment:

    # Set user preferences. 

    I also think adding a space after the # character makes comments easier to read.

    Unused code

    Since this code is unused, it should be removed:

    echo "If you would like to reenter the information type (1)YES; (2)NO" read -r again #Loop back to first echo if again -eq 1 #if [ $again -eq 1 ] 

    Keep track of future enhancements in a plain text file elsewhere.

    Naming

    For variables formed by multiple words, I recommend using snake_case to make them easier to read. For example, use your_name instead of yourname.

    The variable named laptopyn is hard to understand. Again, I recommend using snake_case as well prefixing it with is_, which is common for boolean variables: is_laptop.

    Indentation

    Some of the indentation is misleading. For example:

    echo "$yourname are you sure?" sleep 2 hostnamectl set-hostname "$yourname" echo "Your hostname is set to:" hostname sleep 2 

    When I see the hostname commands indented, expect them to be inside an if/else or loop construct. I would eliminate the indentation there.

    In the following code, I would add indentation to the echo statements since they are inside the if/else construct:

    if [ "$cpu" -eq 1 ] then echo ">>>>Installing Intel drivers" sudo dnf -y swap libva-intel-media-driver intel-media-driver --allowerasing else echo ">>>>Installing AMD drivers" 
    \$\endgroup\$
      3
      \$\begingroup\$

      Consistency

      I find execution slightly difficult to follow because there is a vast difference between what is printed at the start of a section and the corresponding end eg:

       echo ">>>>Sarting with the repo's" echo ">>>>rpmfusion INSTALLED 3/10" 
       echo ">>>>Installing essentials" echo ">>>>google-roboto mozilla-firra,flathpak,unzip,gnome-tweaks,gimp,nautilus-python; INSTALLED 5/10" 

      Instead, you should just echo "Starting something..." and then: "done" (on the same line even). Or "Starting something..." and then: "Finished something" so that we can figure out when a certain section starts and when it ends.

      I would move up the section that adds rpmfusion, just after dnf update, so that it gets installed as soon as possible given that subsequent lines may depend on it.

      Regrouping

      There is one section dedicated to dnf update so this line is an outlier:

      sudo systemctl disable NetworkManager-wait-online.service 

      There should be a dedicated section for networking I think. What you did for Sound & video looks sensible.

      Some lines could be merged because they are logically related:

      sudo dnf install -y gimp sudo dnf install -y gimp-devel 

      Being user-friendly

      Rebooting without prompting for confirmation is not a good idea, because you may have uncommitted work in another window (you never know). In fact you might not need to reboot, unless you've upgraded the Linux kernel. To find out if a reboot is required, here is one way for this OS family.

      Being user-friendly 2

      Don't ask the user for information that can be inferred without too much effort. If you want to determine the CPU type, have a look at the commands lscpu or dmidecode, with some grepping if necessary. Look for specific strings to positively identify the hardware. Here is a nice little challenge for you to up your skills ;)

      Instead of a simple if/else block consider using case/esac even if you have just two options right now eg:

      case $cpu in AMD) echo ">>>>Installing AMD drivers" sudo dnf -y swap mesa-va-drivers mesa-va-drivers-freeworld sudo dnf -y swap mesa-vdpau-drivers mesa-vdpau-drivers-freeworld ;; Intel) echo ">>>>Installing Intel drivers" sudo dnf -y swap libva-intel-media-driver intel-media-driver --allowerasing ;; *) echo -n "Processor unknown, exit!" exit 1 ;; esac 

      If in doubt, we exit. I think this is a desirable approach: if you install the wrong drivers because the user makes a mistake for example, this could mess your graphical configuration and your desktop might not load upon reboot. You'll have to sort out the mess in console.

      Logging

      By the way, it would be nice to log everything to a file somewhere for postmortem analysis. By running your script in a GNU screen or tmux session, you could do that automatically, and you are indeed installing tmux.

      Or do it yourself either on the command line, or by adding instructions to your script. With console redirection in Linux we can copy the output of both stdout and stderr to a log file. If you add this at the top of your script right after the shebang:

      LOG_FILE="/root/fedora-install.log" exec > >(tee -a "$LOG_FILE") 2>&1 

      This should do the trick nicely :) Source. I am not sure this is the absolute best way though.

      shellcheck

      shellcheck has some warnings, I recommend you install and use it to improve your scripts and learn a few things in the process.

      Update or upgrade

      Apparently dnf upgrade is now an alias for dnf update - see: https://github.com/rpm-software-management/dnf5/issues/441

      Alternative tools

      I understand this is a learning exercise to get familiar with Bash, and I think this is a good use case. But for this kind of stuff I would normally suggest Ansible. It has a lot of modules for common and not so common tasks. If for example you need to patch files reliably, this could get tedious in Bash. So far you've done very straightforward things but your requirements may evolve over time.

      Automating the Installation

      But in fact you can also take the logic further and automate the OS install itself. For Fedora this is how it can be done: Automating the Installation with Kickstart. I am only mentioning this for completeness - because I don't think you work as sysadmin for a webhost or something like that. But if you are building a lab, or testing a lot of distros then automation is a must.

      \$\endgroup\$
        2
        \$\begingroup\$
        echo "What is it going to be your hostname?" read -r yourname 

        Since you are using Bash, then your read has options to prompt and to allow editing:

        read -er -p "What is it going to be your hostname? " yourname 
        \$\endgroup\$
          2
          \$\begingroup\$

          Consider putting your functional blocks in functions instead of just indenting them and then just calling each of those if the previous function succeeds, e.g.:

          #!/usr/bin/env bash get_prefs() { #Setting variables according to user preferences. local again=1 while (( again == 1 )); do echo "What is it going to be your hostname?" >&2 read -r yourname echo "Is this a laptop, (1)YES, (0)NO?" >&2 read -r laptopyn echo "the CPU is INTEL(1) or AMD(0)" >&2 read -r cpu echo "If you would like to reenter the information type (1)YES; (2)NO" >&2 read -r again #Loop back to first echo if again -eq 1 done } set_hostname() { #Setting hostname. echo ">>>>Setting you up Champ" >&2 echo "$yourname are you sure?" >&2 sleep 2 hostnamectl set-hostname "$yourname" echo "Your hostname is set to:" >&2 hostname >&2 } update_os() { #Updating OS & disable NtwrkMngr-wait echo ">>>>Starting updates" >&2 sudo dnf -y upgrade sudo dnf -y upgrade --refresh sudo dnf -y update sudo dnf -y groupupdate core sudo fwupdmgr refresh --force sudo fwupdmgr update --force sudo systemctl disable NetworkManager-wait-online.service echo ">>>>You are up to date 1/10 " >&2 } get_prefs && set_hostname && sleep 2 && update_os && sleep 1 ... 

          Also consider redirecting prompts and info messages to stderr as I'm doing above instead of stdout.

          I also moved the applicable sleep statements out of the functions and have them explicit between function calls to aid clarity, etc.

          Regarding echo "Is this a laptop, (1)YES, (0)NO?" >&2 - why not accept Y or N instead of 1 or 0? Ditto for echo "the CPU is INTEL(1) or AMD(0)" >&2 - why not I or A?

          \$\endgroup\$

            Start asking to get answers

            Find the answer to your question by asking.

            Ask question

            Explore related questions

            See similar questions with these tags.