#20990 closed defect (fixed)

Turn on warnings in autotools

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-14
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

The autotools packages are able to show warnings about syntax, cross compilation, and the use of obsolete macros.

Turning on these warnings uncovers some issues.

Child Tickets

Change History (20)

Changed 11 months ago by cypherpunks

Changed 11 months ago by cypherpunks

comment:1 Changed 11 months ago by cypherpunks

Status: newneeds_review

I've attached patches 0001 and 0002 which solves the issues that were uncovered by turning on the autotools warnings. Patch 0003 updates a macro found while working on this ticket. Patch 0004 increases the minimum version of Automake to 1.11.2 for access to the AM_PROG_AR macro. Finally, patch 0005 turns on the warnings to catch future issues.

comment:2 Changed 11 months ago by nickm

Milestone: Tor: 0.3.0.x-final

Changed 11 months ago by cypherpunks

comment:3 Changed 11 months ago by cypherpunks

Added a patch for the changes file and a fixup patch for actually increasing the minimum Automake version which i somehow forgot.

The Tor version mentioned in the changes file was found by searching for the first commit to use the obsolete AC_TRY_COMPILE macro which was commit d060f845f22689e3a3d13ab37f3e0bd0641f7e4c. At the time of that commit the Autoconf version that made it obsolete was already out for 3 years.

comment:4 Changed 11 months ago by teor

"Patch 0004 increases the minimum version of Automake to 1.11.2 for access to the AM_PROG_AR macro"

The other patches seem ok, but we try hard not to increase the minimum automake version.
What would this patch set look like without it?

comment:5 in reply to:  4 Changed 11 months ago by cypherpunks

Replying to teor:

The other patches seem ok, but we try hard not to increase the minimum automake version.

I looked at the table in ticket:17732#comment:2 which shows everything (except Debian Squeeze which is EOL) has a Automake version >= 1.11.2. FWIW Automake 1.11.2 was released in 2011.

What would this patch set look like without it?

Without increasing the Automake version i can't assume AM_PROG_AR is available, therefore i can't use it and Autotools will complain about not using it.

comment:6 Changed 11 months ago by nickm

Keywords: review-group-14 added

comment:7 Changed 11 months ago by nickm

I'm okay about moving automake forward by a couple of years; 1.11.2 was released 5 years ago today.

comment:8 Changed 11 months ago by nickm

But what if we take the approach in my branch bug20990? It replaces your 'AM_PROG_AR' with:

+dnl check for the correct "ar" when cross-compiling.
+dnl   (AM_PROG_AR was new in automake 1.11.2, which we do not yet require,
+dnl    so kludge up a replacement for the case where it isn't there yet.)
+m4_ifdef([AM_PROG_AR],
+         [AM_PROG_AR],
+        [AN_MAKEVAR([AR], [AC_PROG_AR])
+          AN_PROGRAM([ar], [AC_PROG_AR])
+          AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL([AR], [ar], [ar])])
+          AC_PROG_AR])

comment:9 in reply to:  8 Changed 11 months ago by cypherpunks

Replying to nickm:

But what if we take the approach in my branch bug20990?

This is a good approach. I've attached two fixup patches based on your branch. Patch 0001 removes the part about increasing the Automake version which is obsolete now. Patch 0002 fixes an indentation and uses : (aka true) when the AC_CHECK_TOOL macro cannot find a suitable ar.

comment:10 Changed 10 months ago by nickm

Reviewer: nickm

comment:11 Changed 10 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I've added those, and squashed as bug20990_squashed_v2, and merged to master. Thanks!

Note: See TracTickets for help on using tickets.