Opened 2 years ago

Last modified 4 months ago

#19220 new defect

Do not override Makefile user variables

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: gcc-warnings, lorax, 031-deferred-20170425, 032-unreached
Cc: alex_y_xu@… Actual Points:
Parent ID: Points: .5
Reviewer: Sponsor:

Description

This was supposed to be a comment to #19216 but i figured it's better to open a new ticket because it's out of scope.

The real solution to the issue fixed in ticket:19216#comment:4 is to stop assigning to CFLAGS (unless when testing the flag but restore the old CFLAGS after) because it's a user variable which should never be set by the package. Instead the additional parameters should be assigned to a temporary variable (like TOR_CFLAGS or something) which can be AC_SUBSTed and appended to AM_CFLAGS. The same can be said about CPPFLAGS and LDFLAGS. For more information see https://www.gnu.org/software/autoconf/manual/automake.html#Flag-Variables-Ordering.

Overriding user variables also causes issues when trying to run make distcheck in build directories that are configured with --enable-fatal-warnings. It seems the overridden CFLAGS are integrated into the tarball and causes the default Autoconf checks to fail.

Child Tickets

Change History (8)

comment:1 Changed 2 years ago by nickm

Keywords: gcc-warnings added

Sounds like a good idea to me, but we watch out: there's actually a couple of cases where that approach won't work. In any case where the warning triggers on the default autoconf test program, we'll get a false negative given the way that our CHECK_CFLAGS macro works now.

comment:2 Changed 2 years ago by nickm

Keywords: lorax added
Milestone: Tor: 0.3.0.x-final
Points: .5

comment:3 Changed 2 years ago by nickm

I'd be glad to take a patch for this.

comment:4 Changed 20 months ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Triaged out on December 2016 from 030 to 031.

comment:5 Changed 16 months ago by nickm

Keywords: 031-deferred-20170425 added
Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

Triage: batch-defer unowned items of priority Medium or lower to 0.3.2.

comment:6 Changed 15 months ago by nickm

Keywords: triage-out-030-201612 removed

comment:7 Changed 11 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

comment:8 Changed 4 months ago by Hello71

Cc: alex_y_xu@… added
Priority: LowMedium

this is now somewhat more important because travis runs distcheck. it will break when travis eventually upgrades.

what do you mean by "false negative"?

Note: See TracTickets for help on using tickets.