Roll install_deps.sh into pinned_build.sh

[kj4ezj]

From issue 735, this pull request rolls scripts/install_deps.sh into scripts/pinned_build.sh with a guard for operating systems that are not in the Debian family because the install_deps.sh script is not used by anything else. I also sorted the packages, put them each on their own line (for traceability and readability), addressed one linter suggestion about my changes, added some print statements, and updated the pinned build instructions in the README.md.

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

[spoonincode]

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.

[kj4ezj]

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. Ultimately, people may run the whole script as sudo no matter what we do.

[ScottBailey]

lgtm

[greg7mdp]

I had a look and I am not convinced this change is desirable because:

1. 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.
2. 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).
3. the shell script is more complex than the two scripts were before, but I think we actually lost functionality.
4. 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.

[kj4ezj]

I had a look and I am not convinced this change is desirable because:

1. 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.

This deviates from the instructions in the README.md. If the intention is to include install_deps.sh in the unpinned build, the instructions should be updated to reflect that. Right now, the unpinned build instructions provide explicit apt-get commands to run based on what is available by default in the supported versions of Ubuntu after a clean installation. I do not think we should inhibit changes to the pinned build script based on undocumented usage patterns.

2. 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).

I agree it is not ideal to prompt a user for their password each time the script is invoked. However, the purpose of this script is to build binaries using the exact dependencies specified by your team - not to do casual builds. These builds should be performed in a clean environment where we do not expect the dependencies to already exist, such as a clean virtual machine, a docker container, or a CI system. In some of these cases (docker), no password is needed. If the password is needlessly invoked, root permissions are only available to apt-get - not the entire script.

3. the shell script is more complex than the two scripts were before, but I think we actually lost functionality.

I suspect the "lost" functionality is not the intended use-case for these scripts, as described above.

4. 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.

There are multiple warning messages for this purpose, one at the beginning and one when the install_deps function is invoked. Ultimately, we do not support these other platforms. I considered terminating the script altogether on unsupported platforms and requiring them to use a -f or --force flag, but this deviated from the existing behavior and is probably a waste of time.

[spoonincode]

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.

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.

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).

I agree, this does seem like a regression. It's also seemingly at odds with the reasoning behind this change #812 (comment) as now the user does always expect the script to prompt for a password.

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.

Probably out of scope, since the pinned_build.sh script downloads and run x86_64 linux binaries compiled on a specific ubuntu version.

[kj4ezj]

I agree, this does seem like a regression. It's also seemingly at odds with the reasoning behind this change #812 (comment) as now the user does always expect the script to prompt for a password.

I can add code to detect if the dependencies exist, but I just don't think this is worth our time - especially for a script you told me not to invest too much time into because you plan to delete it.

I rolled these together because having two separate scripts cost me several hours of time because I did not read the instructions that in the README.md I wrote (lol), so I did not know I needed to run the install_deps.sh script before I could run pinned_build.sh. It doesn't make sense - why have a pinned build script at all if it doesn't do everything it needs to do? At the very least, it should look for missing dependencies and warn about the other script...at which point, we are back to me writing code that could just do that to bypass the install_deps function within this script, which I can do.

I think our goal should be focusing on making these processes easier for downstream consumers to do, above some ideological principle like "it is bad to ask for sudo." I would double-down on this stance after the last BP call. Our process is already so complicated that people do not update.

[matthewdarwin]

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

[greg7mdp]

I still don't see this change as an improvement. 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.
So I'll just refrain from approving this PR, but if someone else (besides Scott) find this an improvement, by all means please approve.

[kj4ezj]

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

I found that this may be the fastest way to move forward. I have gone ahead and pushed changes that look for the list of dependencies and only run the code invoking apt-get if there are missing dependencies. This is still guarded by the OS checks, and the list of dependencies is not duplicated.

[kj4ezj]

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.

They aren't simple, though. It does not make sense to have two different scripts for a pinned build. It is also not easier to debug - I spent over two hours trying to debug missing dependencies because I did not read my own instructions in the README.md that explain you need to run this wholly unrelated script first, and the feedback I got from pinned_build.sh did not make it clear this was the problem and did not provide useful feedback for debugging. Yes, I was the idiot there since I wrote the pinned build instructions some months ago, but I am missing the part where that was somehow easier to debug.

[spoonincode]

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

This doesn't look sufficient, as just installing and uninstalling the package still returns 0 exit code, for example

# dpkg -l cmake ; echo $?
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name           Version      Architecture Description
+++-==============-============-============-=================================
un  cmake          <none>       <none>       (no description available)
0

[kj4ezj]

This is really good feedback! I switched from dpkg -l to dpkg -s, which I verified does not have the problematic behavior on Ubuntu 22.04:

Thank you.

[spoonincode]

I think our goal should be focusing on making these processes easier for downstream consumers to do

💯 but the solution is not more complex build scripts.

[greg7mdp]

They aren't simple, though

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. Each does exactly what its name implies, no more, no less. They don't have to jump through hoops to check if deps are already installed, or whether to run sudo.

[kj4ezj]

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.

I did not use any BASH syntax that was not already present. The try() function, the use of functions in general, the use of variables, if statements, accessing /etc files, and other "advanced" BASH syntax was already there. This is an absurd claim.

Each does exactly what its name implies, no more, no less. They don't have to jump through hoops to check if deps are already installed, or whether to run sudo.

Except pinned_build.sh did NOT do exactly what the name implied. Perhaps it did exactly what the name implied to you, but it did not do what the name implied to me, and I suspect it would not do what the name implies to other users.

For example, pinned_build.sh installed clang, LLVM, and Boost - but not any of the other dependencies. So which is it, should those steps be moved into install_deps.sh, or do dependency installations belong in pinned_build.sh? It cannot be both if we are going by script names.

---

In any case, this does not matter because I have added code to detect whether dependencies are missing or not in a Debian-family OS, then only invoke apt-get as-needed, as requested.

[kj4ezj]

Closing because this body of work has been split into three smaller pieces to help address some of the contention and make forward-progress:

• Pull Request 847 - Restore libcurl4-openssl-dev
• Pull Request 849 - Fix README.md Formatting
• Pull Request 857 - Simplify Pinned Build Scripts

Please continue the conversation there.