build: Fail early and show actionable messages if autogen deps are missing #25523

pull bubelov wants to merge 1 commits into bitcoin:master from bubelov:autogen-assert-deps changing 1 files +19 −4

bubelov commented at 12:27 PM on July 1, 2022: contributor

All listed deps are necessary for autogen.sh to do its job. It currently shows cryptic and non-actionable error messages if some of those deps are missing
in autogen.sh:17 in 6f611ebfe8 outdated
```
  13 | +      return 1
  14 | +    fi
  15 | +  done
  16 | +}
  17 | +
  18 | +assert_deps aclocal autoreconf libtool pkg-config
```
hebasto commented at 12:34 PM on July 1, 2022:

libtool package has libtoolize binary:

assert_deps aclocal autoreconf libtoolize pkg-config

But message "configuration failed, please install libtoolize first" still useless.
bubelov commented at 12:49 PM on July 1, 2022:

@hebasto yep, it seems like libtool binary is not always present so it can't be used. It's present in Arch libtool but it's not provided by Debian

Without this check, it looks like this:

Can't exec "libtoolize": No such file or directory at /usr/share/autoconf/Autom4te/FileUtils.pm line 293. autoreconf: error: libtoolize failed with exit status: 2

So maybe it would still look a bit cleaner
DrahtBot added the label Build system on Jul 1, 2022
jarolrod commented at 2:54 AM on July 27, 2022: member

A clean commit history is required. Please squash your commits, see: CONTRIBUTING.md#squashing-commits
bubelov force-pushed on Jul 27, 2022
bubelov commented at 3:58 AM on July 27, 2022: contributor

@jarolrod done
fanquake commented at 10:29 AM on August 1, 2022: member

This PR doesn't currently work, because libtoolize is glibtoolize on macOS. Overall I think this change is ~0.
Fail early if autogen deps are missing 862dc6f20e
bubelov force-pushed on Aug 2, 2022
bubelov commented at 6:11 AM on August 2, 2022: contributor
@fanquake thanks for the tip, I fixed that. Although I agree that this change is minor, I believe it's a net positive for the following reasons:
- The current version of this script already checks for certain deps, namely autoreconf, and it fails early in case it's missing. Adding other deps or removing this check would make this script more consistent
- Failing early is usually a good practice, it saves time and de-clutters output
- Every missing dep currently causes inconsistent and cryptic output. Showing the same error template with clear and actionable message is especially helpful for people who aren't familiar with building C/C++ software and for people who used other build systems
- It's tempting to assume that those packages are always present or that everyone knows about them, but it's not always the case. For instance, many cloud and container images are intentionally minimized, so the situation when some crucial deps are missing isn't that uncommon
- Listing all the deps explicitly improves visibility, it makes it easier to understand what Bitcoin Core relies on at every point in the build pipeline
DrahtBot commented at 11:14 AM on September 23, 2022: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

No conflicts as of last run.
achow101 commented at 7:40 PM on October 12, 2022: member

Are you still working on this?
bubelov commented at 2:45 AM on October 13, 2022: contributor

@achow101 it's done, there is nothing left to add

in autogen.sh:23 in 862dc6f20e

  20 | +# libtoolize is glibtoolize on macOS
  21 |  if [ -z "${LIBTOOLIZE}" ] && GLIBTOOLIZE="$(command -v glibtoolize)"; then
  22 |    LIBTOOLIZE="${GLIBTOOLIZE}"
  23 |    export LIBTOOLIZE
  24 | +else
  25 | +  assert_deps libtoolize

TheCharlatan commented at 8:46 AM on January 30, 2023:

This does not instruct the user to look for glibtoolize on macOS.

TheCharlatan changes_requested
fanquake commented at 12:36 PM on February 16, 2023: member

Thanks. However I don't think we are going to merge this. We'll also soon be migrating away from autotools & friends.
fanquake closed this on Feb 16, 2023
bitcoin locked this on Feb 16, 2024

Contributors

Labels

build: Fail early and show actionable messages if autogen deps are missing #25523

Reviews

Conflicts