Simplify Pinned Build Scripts

[kj4ezj]

From issue 735, this pull request simplifies the process of performing a pinned build by merging scripts/install_deps.sh into scripts/pinned_build.sh.

This is simpler for our customers because now they only have to run a single command that calls a single BASH script to perform the entire pinned build process. This is simpler for our team to maintain because all of the code is in one place, we no longer need to maintain multiple scripts, and it is unambiguous how the pieces fit together.

The aptitude code only runs on Debian-family operating systems, it only runs if package dependencies are missing, it only invokes sudo if it is not running as root already, and it prints bold yellow warnings to the terminal for unsupported non-Debian-family users so they know this step is being skipped, in addition to the print statements already at the top of the script. This is all documented in the README.md.

This code has been checked with multiple BASH linters.

bashate -i E006
shellcheck -x -f gcc

I believe I have addressed all of the concerns my fellow engineers and community members brought up during my previous attempt.

See Also

1. Pull Request 833 - Fix Pinned Build on Ubuntu 22.04
   • Reverts pull request 799
2. Pull Request 812 - Pinned Build Script Linter Changes
3. Pull Request 835 - Change ASCII Art Banner in Build Script
4. Pull Request 832 - Roll install_deps.sh into pinned_build.sh
5. Pull Request 847 - Restore libcurl4-openssl-dev
6. Pull Request 849 - Fix README.md Formatting
7. Pull Request 870 - Pinned Build Script Backport Omnibus
8. Pull Request 857 - Simplify Pinned Build Scripts
9. Pull Request 811 - Pinned Build Script Changes

[kj4ezj]

This pull request contains part of the changes I tried to introduce in pull request 832. I believe I have addressed everyone's concerns from that pull request. I will lay out the concerns I read there and explain how I have addressed each and every one individually to try to head-off contention and to speed up peer review.

See More

1. The downside with this is that it encourages users to run the entire script as root, where really only just the apt-get bits need to be run as root.

   That is a really good point! I added code that prepends sudo to the apt-get commands if the current user is not root. If the current user is root, like in a docker container, sudo is excluded. I hope you like it! I also updated the pinned build instructions in the README.md to reflect my changes, including a note that the script does not need to be invoked using sudo.

   I have further emphasized this script should not be run with sudo in the README.md, expanded on why specifically that is the case, and added a few examples of how one might run the script which all exclude sudo.

   They don't have to jump through hoops to check [...] whether to run sudo.

   The user never runs sudo, this is now documented in the README.md.

2. install_deps.sh can be useful even if you don't want to do the pinned build. I used it myself to install the dependencies, even though I was doing the unpinned build.

   A Principle Engineer addressed this for me.

   That really isn't the purpose of the script and it's missing some dependencies required for unpinned builds (boost, llvm). It's more of a install_deps_for_pinned_build.sh script.

   In my opinion, we should really be following the instructions in the README.md or the Tool Install Guide to install unpinned dependencies so that we know if they are broken. However, the dependencies now exist in a flat file list at scripts/pinned_deps.txt, which is both easier to maintain and also allows you to do this.

   sudo xargs -a scripts/pinned_deps.txt aptitude install -y

   You can use this to create a BASH alias in your ~/.bashrc...

   alias eos-deps='sudo xargs -a ~/Work/antelope/leap/scripts/pinned_deps.txt aptitude install -y'

   ...or program it into a macropad, which is portable across containers and virtual machines. One of these solutions should meet your needs.

3. it now requires sudo permissions every time pinned_build.sh is run, instead of just when running install_deps.sh (which is typically run only once).

   Fixed! Now, sudo is only invoked if and only if there are missing package dependencies, and the script is not running as root like it might be in a docker container.

4. the shell script is more complex than the two scripts were before

   The original two scripts are significantly simpler than your new version. They are easier to understand, esp. if the user doesn't know the bash scripting language.

   Code complexity is a nebulous idea that is difficult to talk about in objective terms, so the ENF Engineering team ratified this document, which attempts to define twelve clear demarcation points at which a BASH script has become too complex for our team to accept or to continue to maintain. This script does not violate any of those twelve points so our team has decided as a group, in advance and including yourself, that this level of complexity is acceptable.

5. I think having the install_deps step separate may be better for people building on other platforms, like MacOS, as it makes it obvious that they have to install dependencies themselves, and the script is easier to understand.

   We do not support any platforms besides Ubuntu. However, as a show of good-faith, I have added OS detection and the script prints bold yellow warnings to the terminal for unsupported non-Debian-family users so they know this step is being skipped, in addition to the print statements already at the top of the script.

6. in pinned_build.sh don't prompt me to run sudo if there is nothing I need to install.

   Done! It will only try to run apt-get if dependencies are missing.

7. I'd rather have two dead simple scripts, install_deps.sh and pinned_build.sh, each doing its clear task, rather than one more complex script trying to do everything for me. In case of problems, it is harder to figure out what to to imho.

   I can't speak to personal preferences with respect to implementation details but, to the last point about it being more difficult to debug...fixed! I have taken several steps to make debugging easier. My priority when writing code is almost always to make it as easy to debug as possible because humans are much more expensive than compute. I added the warning statements I talked about above, print statements before and after the package install step, the try() function prints the command it is about to run, and it now returns the exit status of that command on failure instead of returning a generic exit status.

8. you can put the command directly inside the if, so you don't need a MISSING_DEPS variable.

   Done.

9. [dpkg -l] doesn't look sufficient, as just installing and uninstalling the package still returns 0 exit code, for example

   Fixed, switched to dpkg -s.

10. They don't have to jump through hoops to check if deps are already installed

    Fixed! This is done automatically.

11. I'm not really convinced [...] TZ [...] should be here at all. Who are we to decide what time zone a user's box should be initialized to? [...] This should just be left default and the CI can override it.

    Removed. I agree, tzdata defaults to UTC so it was a no-op anyways.

12. I'm not really convinced [...] the -y should be here at all. [...] Why should we not allow a user to interactively confirm what apt-get is about to do? This should just be left default and the CI can override it.

    Removed. The apt-get command happens at the beginning of the script anyways, before someone might walk away, and there is only one prompt. I have also added instructions to the README.md that explain how to suppress this prompt, if the user wishes to do so.

13. I'm not really convinced [...] DEBIAN_FRONTEND [...] should be here at all.

    I have verified this does not suppress the [Y/n] prompts from aptitude. The tzdata prompts are frankly confusing, intrusive, needy, and annoying. I think we should allow users to set the timezone using the TZ environment variable, like we did before.

[ScottBailey]

Suggested change

	If you want to by-pass the `[Y/n]` prompt from aptidue, you can pass `-y` to the script like this.
	If you want to by-pass the `[Y/n]` prompt from apt-get, you can pass `-y` to the script like this.

[kj4ezj]

Changed.

[ScottBailey]

aptitude != apt-get

Otherwise, lgtm

[ScottBailey]

lgtm

[spoonincode]

Not sure how that works. sudo doesn't consume from stdin? I don't think -y is correct here anyways, from this test

echo -y | apt-get upgrade
Reading package lists... Done
Building dependency tree
Reading state information... Done
Calculating upgrade... Done
The following packages will be upgraded:
  base-files libgmp10 libgnutls30 libpam-modules libpam-modules-bin libpam-runtime libpam0g libpcre2-8-0
  libsystemd0 libudev1 login passwd perl-base tar zlib1g
15 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
Need to get 4893 kB of archives.
After this operation, 14.3 kB of additional disk space will be used.
Do you want to continue? [Y/n] Abort.

It seems like you may want to allow the user to pass -y as a script argument, and then forward that through to apt-get

[kj4ezj]

Good catch, -y was not correct! I have amended it to just y. I tested this on Ubuntu 22.04 without sudo.

As for sudo passing STDIN to apt-get, it worked for me.

From ChatGPT:

When you run echo 'y' | sudo cmd, the echo command sends the character y to its standard output (stdout), which is then piped to the standard input (stdin) of the sudo command. The sudo command then runs cmd with its stdin connected to the pipe.

For example, if cmd is the apt-get command, and you run echo 'y' | sudo apt-get install package, the apt-get command will read the y character from its stdin and assume "yes" as the answer to any prompts.

I had considered (and would prefer) the pattern of passing -y as an argument to the BASH script, but it doesn't seem user-friendly to require the -y argument to be, say, argument 4 and only argument 4 because most tools do not work that way. I could've written code to parse the arguments no matter where the -y flag is in the argv, but other peer reviewers were already complaining about code complexity so I went with the simplest implementation.

[spoonincode]

afaik sudo is not guaranteed to be installed

[ScottBailey]

afaik sudo is not guaranteed to be installed

Truth. And not everyone likes sudo, either. It might be appropriate to test for sudo and report an error if it's missing.

[kj4ezj]

Done.

[spoonincode]

I'd prefer to avoid adding additional complexity to build scripts (and, really, the existence of build scripts at all); I'm deferring to others on this particular one.