search

the-tcpdump-group / tcpslice

tcpslice concatenates multiple pcap files together, or extracts time slices from one or more pcap files.
63 stars 22 forks source link

Allow disabling optional extra libraries #12

Closed fenner closed 2 years ago

fenner commented 2 years ago

In our build system, a package may be built in any order, meaning that when built today libnids may not be present, and when built tomorrow libnids may be present, just because of someone else's dependency.

In order to create repeatable builds, we disable optional libraries that we don't explicitly want, so this pull request adds AC_ARG_WITH statements to add --without-foo for libnids, libosipparser2, libooh323c.

Technically, you are supposed to add an additional check: if the user specified --with-libfoo and the AC_CHECK_LIB didn't find it, then you're supposed to error out. I don't find that use case necessary, but will add it if maintainers think it's appropriate.

Note that even though we are both using things that call themselves autoconf 2.69, I had to edit the generated configure script to retain the runstatedir support, and to keep LARGE_OFF_T around 32-bits-ish (my version wanted to make it 63-bits-ish). Is there a set of patches that this project prefers to apply to autoconf?

infrastation commented 2 years ago

Thank you for preparing this change, it looks good on paper (to me at least). Other opinions are welcome.

Regarding runstatedir and LARGE_OFF_T, these chunks depend on Autoconf variety (Guy's Autoconf 2.69 usually does not generate these, mine 2.69 usually does, and 2.71 produces even more different results). I usually leave those differences out of the commits by using git add -p or omitting configure at all.

infrastation commented 2 years ago

On my PC, which has all three optional libraries available, this works as described. The only potential improvement that caught my eye during the test was the new flags descriptions: perhaps it would be useful to tell that --without-libnids automatically disables the other two libraries (and maybe make the letter case consistent too):

Optional Packages:
  --with-PACKAGE[=ARG]    use PACKAGE [ARG=yes]
  --without-PACKAGE       do not use PACKAGE (same as --with-PACKAGE=no)
  --without-gcc           don't use gcc
  --with-system-libpcap   don't use local pcap library
  --without-libnids       Do not use libnids even if present
  --without-libosipparser2
                          Do not use libosipparser2 even if present
  --without-libooh323c    Do not use libooh323c even if present

Let's merge this tomorrow.

guyharris commented 2 years ago

Guy's Autoconf 2.69 usually does not generate these

That's 2.69 downloaded from ftp.gnu.org and configured and installed on macOS, with no patches.

mine 2.69 usually does

Is that a version that comes with a Linux distribution? Perhaps they've made patches.

2.71 produces even more different results

Perhaps this is the universe's way of saying that we shouldn't commit the results to autoconf to the repository, but should generate it as part of the process of generating a release tarball, so that what's in the release tarball comes from a known version of autoconf.

Either that, or if possible, have something on a machine running a known version of autoconf regenerate the configure script when a change to configure.ac is committed, and commit the results.

infrastation commented 2 years ago

The last option is not far from the current modus operandi: your Autoconf has been the reference generator for a while.

guyharris commented 2 years ago

your Autoconf has been the reference generator for a while.

The fact that I've probably been the person who's changed the configure scripts most often shouldn't render the autoconf on my machine the official standard.

The autoconf on my Ubuntu 20.04 VM claims to be 2.69, but generates results that might be more like what Bill's seeing. Either it's been patched, or, for some reason, 2.69's own autoconf script decides to do things differently on macOS 11 and Ubuntu 20.04, which would Not Be A Good Sign - it shouldn't matter what OS you run a given version of autoconf on; as long as it's the same version of autoconf on both platforms, the resulting script should behave the same.

infrastation commented 2 years ago

Is that a version that comes with a Linux distribution? Perhaps they've made patches.

My current Autoconf comes from a Ubuntu 20.04 package, which has version 2.69-11.1, its source package includes a .debian.tar.xz file with 8 patches, one of which is add-runstatedir.patch dated 12 Sep 2013 and another is avoid-undefined-behaviour-for-32bit-off_t.patch dated around 2014, which explains why --runstatedir and LARGE_OFF_T respectively have been popping up in my scope since Ubuntu 14.04 (and possibly before that, when I still used Fedora, if Fedora applied these changes too at the time; it is difficult to remember exactly).

Not long ago I ran into similar differences between different flavours of Autoconf 2.13, which could not be easily resolved with git add -p, so it took to find a Linux distribution that could produce the right configure (see the end of commit message in OSGeo/grass#1719). It would not be surprising to find out that there is more than one flavour of Autoconf 2.71. So I prefer not to touch it if possible and I trust you know better, especially that tcpslice provides a convenient playground for build system improvements.

guyharris commented 2 years ago

avoid-undefined-behaviour-for-32bit-off_t.patch

That's a fix for Debian bug 742780.

For what it's worth, I tested the libpcap 1.10.1 release tarball, which appears to have a configure script generated by an autoconf with that change, and an 1.11.0-PRE-GIT release tarball, generated on my Mac with a configure script produced by an unpatched 2.69, on a 32-bit Ubuntu 18.04 virtual machine, and they both put #define FILE_OFFSET_BITS 64 into config.h, so I'm not sure on what platforms the configure script fails to do the right thing to get 64-bit file offsets.

guyharris commented 2 years ago

add-runstatedir.patch

That's a "mail message patch", and the message begins with

From a197431414088a417b407b9b20583b2e8f7363bd Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 12 Sep 2013 15:11:29 -0600
Subject: [PATCH] AC_INIT: add --runstatedir option to configure

http://lwn.net/Articles/436012/ documents that many distros
are now preferring to use /run rather than /var/run for
storage of pid files and other per-process temporary files
that must not be cleaned out during arbitrary TMPDIR sweeps.
As such, the GNU Coding Standards were recently changed to
recommend a new configure option to make it easy to choose
this directory at configure time.  This patch adds support
for the option to all configure scripts built by autoconf.
guyharris commented 2 years ago

Is there a set of patches that this project prefers to apply to autoconf?

I suppose we could recommend something like "2.69 plus the current Ubuntu patches".

fxlb commented 2 years ago

FYI, Debian stable has 2.69-14 with the following 9 patches:

atomic.patch
stricter-versioning.patch
texinfo.patch
avoid-undefined-behavior-for-32bit-off_t.patch
AM_PROG_LIBTOOL.patch
add-runstatedir.patch
unescaped-left-brace-warning-fix.patch
mmap-leak-fix.patch
remove-build-date-from-autoconf.texi-clo.patch
guyharris commented 2 years ago

atomic.patch

Description: Make autom4te atomically replace its output file.
 Upstream is not interested in this patch; see Forwarded: URL.
Forwarded: http://lists.gnu.org/archive/html/autoconf-patches/2008-08/msg00032.html
Bug-Debian: http://bugs.debian.org/221483
Author: Ben Pfaff <pfaffben@debian.org>
Last-Update: 2006-05-25

Probably doesn't have to be applied, but if you have a script to download the 2.69 source and the debian stuff and apply the patches from the latter, it'd be more work not to apply it.

stricter-versioning.patch

Description: Invalidate the Autom4te cache more aggressively.
 Upstream only invalidates the Autom4te cache if it is older than
 auto4mte, but this only works well if the autom4te binary date
 reflects the date when it was installed.  In debian, the autom4te
 binary date is instead the date when it was built, which doesn't
 guarantee that caches will be properly invalidated.
 .
 Upstream may be interested, but requested some changes that have not
 yet been made; see the Forwarded: URL.
Forwarded: http://lists.gnu.org/archive/html/autoconf/2006-05/msg00127.html
Bug-Debian: http://bugs.debian.org/219621
Author: Ben Pfaff <pfaffben@debian.org>
Last-Update: 2003-11-10

Ditto.

texinfo.patch

Description: Fix build error with texinfo 5.1.
 Backported from a patch applied upstream.
Forwarded: http://old.nabble.com/CVS-Texinfo-fails-to-build-the-Autoconf-manual-%28from-git%29-td34167051.html
Bug-Debian: http://bugs.debian.org/711297
Author: Patrice Dumas, Eric Blake
Last-Update: 2013-08-11

Only of interest if you care about reading the texinfo documentation, but, again, if a robot is applying the patches, it's more work not to apply it.

avoid-undefined-behavior-for-32bit-off_t.patch

See previous comment.

AM_PROG_LIBTOOL.patch

Description: Make autoreconf recognize AM_PROG_LIBTOOL too
 .
 during the "getting the main packages ready"-campaign for ppc64el I
 noticed that dh-autoreconf doesn't work if a package uses
 AM_PROG_LIBTOOL but works with AC_PROG_LIBTOOL though both makros
 should behave the same (and are both deprecated) according to
 http://www.gnu.org/software/libtool/manual/html_node/LT_005fINIT.html
 A sample package for this is version 0.4.5+cvs20030824-7 of smpeg (I
 will upload an NMU now for that package, because it prevents the build
 of kde4libs).
 .
 The following patch changes this behaviour, and would save quite many
 sourceful uploads, so I'd ask you to check the fix and if applicable
 upload it soon. That would really be a great help for this / all
 future ports of Debian (though of course packages should be fixed to
 not use deprecated macros, but that's something else).
Author: Andreas Barth <aba@ayous.org>
Bug-Debian: http://bugs.debian.org/759739
Forwarded: yes

We don't use libtool, but ... robot ....

add-runstatedir.patch

See previous comment.

unescaped-left-brace-warning-fix.patch

Description: bin/autoscan.in: Fix "unescaped left brace" warning from Perl.

Requested by Paul Wise <pabs@debian.org>.  Closes: #818855.
Author: Ben Pfaff <pfaffben@debian.org>
Bug-Debian: https://bugs.debian.org/818855

I haven't seen the warning, but ... robot ....

mmap-leak-fix.patch

From 09b6e78d1592ce10fdc975025d699ee41444aa3f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 5 Feb 2016 21:06:20 -0800
Subject: [PATCH] Fix memory leak in AC_FUNC_MMAP

* lib/autoconf/functions.m4 (AC_FUNC_MMAP): Fix memory leak
in test case, found by configuring with gcc -fsanitize=address.

Oh crap, you have to worry about ASAN stuff if you configure with CFLAGS=-fsanitize=address LDFLAGS=-fsanitize=address. But we don't currently test for mmap() (libpcap uses it on some platforms, but only onOSes that are known to have it). Then again, ... robot ....

remove-build-date-from-autoconf.texi-clo.patch

From: Vagrant Cascadian <vagrant@reproducible-builds.org>
Date: Fri, 25 Dec 2020 08:39:37 +0000
X-Dgit-Generated: 2.69-14 f658040a2f1dfdd25a92a321bb64540657c0b0be
Subject: Remove build date from autoconf.texi (Closes: #978054).

autoconf.texi is used to generate .pdf, .ps, .info and .html
documentation shipped with the package.

See "Timestamps are best avoided":

  https://reproducible-builds.org/docs/timestamps/

Not one of the most severe problems in the universe, but ... robot ....

fxlb commented 2 years ago

The 8 first are the same than in Ubutu focal (20.04LTS).

infrastation commented 2 years ago

Thank you for the analysis. Could you let me know the preferred course of action for this pull request?

infrastation commented 2 years ago

Meanwhile you might want to update the change log as well.

guyharris commented 2 years ago

Could you let me know the preferred course of action for this pull request?

I'd say "just merge it", as the changes to the configure file in the PR seem to be limited to changes that result from the changes to configure.ac plus some whitespace changes that shouldn't affect the behavior of the script - as Bill said, "I had to edit the generated configure script to retain the runstatedir support, and to keep LARGE_OFF_T around 32-bits-ish", so that stuff didn't change.

That doesn't involve a resolution of the issue of what to do about the existence of different versions of autoconf, including different versions of autoconf 2.69, for all three of our projects; perhaps that should be discussed on the mailing list.

infrastation commented 2 years ago

I have rebased this change on the master branch, copied the use case description into the commit message and merged. Thank you!