Opened 5 months ago

Closed 4 months ago

#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 5 months ago by cypherpunks

Changed 5 months ago by cypherpunks

comment:1 Changed 5 months ago by cypherpunks

  • Status changed from new to needs_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 5 months ago by nickm

  • Milestone set to Tor: 0.3.0.x-final

Changed 5 months ago by cypherpunks

comment:3 Changed 5 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 follow-up: Changed 5 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 5 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 5 months ago by nickm

  • Keywords review-group-14 added

comment:7 Changed 5 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 follow-up: Changed 5 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 5 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 5 months ago by nickm

  • Reviewer set to nickm

comment:11 Changed 4 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

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

Note: See TracTickets for help on using tickets.