secp256k1 icon indicating copy to clipboard operation
secp256k1 copied to clipboard

feat(build): add autoreconf dependency check

Open epiccurious opened this issue 1 year ago • 4 comments

Add check for autoreconf dependency to autogen.sh and refactor -if to long forms of optional arguments.

Impact

  • Add a functional change to improves error handling to provide a more helpful message if autoreconf is not found.
  • Add a non-functional refactor to change the autoreconf optional arguments from -if to --install --force.
  • These changes align with the format of autogen.sh on the bitcoin repo

Background

10 years ago: Created the entire file with no other updates.

epiccurious avatar Aug 13 '24 19:08 epiccurious

  • Add a functional change to improves error handling to provide a more helpful message if autoreconf is not found.

On my system, the current error message is already quite descriptive:

$ ./autogen.sh 
./autogen.sh: 3: autoreconf: not found

hebasto avatar Aug 13 '24 19:08 hebasto

the current error message is already quite descriptive

I agree, but the error message could be made even more helpful for those less familiar with the build process.

Since autoreconf is a part of the autoconf package, directing users to install autoconf (rather than just indicating that autoreconf is missing) makes it clearer which package to install.

autoconf is named consistently across popular Unix-based package managers, so this saves a step for users who try to install autoreconf:

apt-get install autoconf
dnf install autoconf
pacman -S autoconf
install autoconf
zypper install autoconf
apk add autoconf
nix-env -iA nixpkgs.autoconf
emerge dev-util/autoconf

epiccurious avatar Aug 14 '24 13:08 epiccurious

the current error message is already quite descriptive

I agree, but the error message could be made even more helpful for those less familiar with the build process.

Since autoreconf is a part of the autoconf package, directing users to install autoconf (rather than just indicating that autoreconf is missing) makes it clearer which package to install.

autoconf is named consistently across popular Unix-based package managers, so this saves a step for users who try to install autoreconf:

apt-get install autoconf
dnf install autoconf
pacman -S autoconf
install autoconf
zypper install autoconf
apk add autoconf
nix-env -iA nixpkgs.autoconf
emerge dev-util/autoconf

The suggested approach is incomplete. Wouldn't it be better to document build prerequisites, which also include automake, libtool, etc?

hebasto avatar Sep 25 '24 17:09 hebasto

The suggested approach is incomplete. Wouldn't it be better to document build prerequisites, which also include automake, libtool, etc?

Yes, I think so, and this should probably simply go to the README then.

real-or-random avatar Sep 25 '24 20:09 real-or-random