Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1773 closed defect (fixed)

Use curly braces for variable expansion in makefiles.

Reported by: ln5 Owned by: ln5
Priority: Low Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: nickm, sebastian, ioerror Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Building doc when `--disable-asciidoc' is broken on NetBSD. From 398012 in linus/makefile-var-exp:

  • doc/Makefile.am: Change $(VAR:MOD) to ${VAR:MOD} -- make(1) on NetBSD substitutes '$(:x)' to 'x' rather than the empty string. This bites us in doc/ when configured with `--disable-asciidoc'. Curly braces should work in all implementations of make(1) but this patch changes only the places where we use the VAR:MOD expansion.

https://gitweb.torproject.org/linus/tor.git/shortlog/refs/heads/makefile-var-exp

Child Tickets

Attachments (1)

a (865 bytes) - added by ln5 9 years ago.
Replace := with ::=.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by ln5

Owner: set to ln5
Status: newaccepted

comment:2 Changed 9 years ago by ln5

Status: acceptedneeds_review

comment:3 Changed 9 years ago by erinn

Reporter: changed from linus to ln5

comment:4 Changed 9 years ago by Sebastian

Looks good to me. Thanks for finding this! Testing on OS X and a few different linux distributions didn't reveal any problems.

comment:5 Changed 9 years ago by arma

Looks great, except it's missing a changes file. E.g. a changes/build-on-netbsd which says

o Minor bugfixes:

  • Resume building on NetBSD when --disable-asciidoc is chosen during ./configure.

comment:6 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good to me too. The commit message is wonky, though: in Git, the first line of a commit message is supposed to be a short summary of the commit log, so that tools like "git shortlog" will work. Cleaned that up, added a 'changes' file, and merged.

comment:7 Changed 9 years ago by ln5

Resolution: fixed
Status: closedreopened

Replacing parentheses with curly braces didn't really cut it.

What it takes is to change ':=' to '::='.
GNU Make seems happy enough with this but Automake gets annoyed by it and omits a warning for every occurrence in Makefile.am:

asciidoc_files
non-POSIX variable name

which is a bit misleading since it's not the variable _name_ it's upset about but rather the '::='.

Appending a patch. Let me know if we want this or if we rather should try to find some other way of doing this.

Changed 9 years ago by ln5

Attachment: a added

Replace := with ::=.

comment:8 Changed 9 years ago by nickm

Status: reopenedneeds_review

To be clear, this isn't a gmake-only thing, is it? We try not to require gnu make.

If it does work with gmake and non-gnu makes, then I say "merge".

comment:9 Changed 9 years ago by ln5

It has nothing to do with GNU Make. One could argue that Automake is wrong here but I haven't looked into it. They seem to warn like this when you do non-portable things if I understand things correctly, which might be a hint that this is not a good idea. I have tested with GNU make and BSD make (netbsd) but only with a recent version of each.

Another option, as discussed with Sebastian on #tor-dev the other day, would be to set the makefile variables using this expansion thing within the 'if USE_ASCIIDOC' clause. A bit cludgy, admittedly.

I've pushed the s/:=/::=/ change to my makefile-var-exp in case you'd like to merge. I hope that branch is in proper shape, please tell if it's not. We don't need another changes file, do we?

comment:10 Changed 9 years ago by nickm

Hm. With OSX 10.5 [GNU Make 3.81] and this patch, I get the following warnings:

Makefile:556: warning: overriding commands for target `tor'
Makefile:546: warning: ignoring old commands for target `tor'
Makefile:556: warning: overriding commands for target `tor-gencert'
Makefile:546: warning: ignoring old commands for target `tor-gencert'
Makefile:556: warning: overriding commands for target `tor-resolve'
Makefile:546: warning: ignoring old commands for target `tor-resolve'
Makefile:556: warning: overriding commands for target `torify'
Makefile:546: warning: ignoring old commands for target `torify'
Makefile:566: warning: overriding commands for target `tor'
Makefile:556: warning: ignoring old commands for target `tor'
Makefile:566: warning: overriding commands for target `tor-gencert'
Makefile:556: warning: ignoring old commands for target `tor-gencert'
Makefile:566: warning: overriding commands for target `tor-resolve'
Makefile:556: warning: ignoring old commands for target `tor-resolve'
Makefile:566: warning: overriding commands for target `torify'
Makefile:556: warning: ignoring old commands for target `torify'
Makefile:565: target `tor' given more than once in the same rule.
Makefile:565: target `tor-gencert' given more than once in the same rule.
Makefile:565: target `tor-resolve' given more than once in the same rule.
Makefile:565: target `torify' given more than once in the same rule.

comment:11 Changed 9 years ago by ln5

Weird. I get nothing like that with the exact same GNU Make on a system reporting itself as

$ uname -v
Darwin Kernel Version 9.8.0: Wed Jul 15 16:55:01 PDT 2009; root:xnu-1228.15.4~1/RELEASE_I386

I'm sure you did the usual autogen.sh and configure dance, right?
I'll try to reproduce on OSX 10.5 as soon as I get hold of one of these.

comment:12 Changed 9 years ago by nickm

Yup. When I do autogen.sh, I get
{{{[48]% sh autogen.sh
doc/Makefile.am:22: asciidoc_files:: non-POSIX variable name
doc/Makefile.am:24: asciidoc_files:: non-POSIX variable name
doc/Makefile.am:26: asciidoc_files:: non-POSIX variable name
doc/Makefile.am:31: asciidoc_files:: non-POSIX variable name
doc/Makefile.am:35: asciidoc_files:: non-POSIX variable name
}}}
then I configure, then "cd doc && make" gets me the warnings listed above.

My autoconf is 2.61; my automake is 1.10.

comment:13 Changed 9 years ago by ln5

Right, I see it too. Sorry about that. So, this patch is obviously a real sad one. :-(

Let's forget about this and prepare something more cludgy. But also working.

comment:14 Changed 9 years ago by ln5

Branch build-wo-asciidoc-on-bsd in my repo reverts the curly braces changes (since they didn't help anyway) and fixes the issue in a less elegant (but more working) way.

comment:15 Changed 9 years ago by ln5

It's been tested with and without --disable-asciidoc on Linux and NetBSD, both make and make distcheck.

Please note that make distcheck is broken in master when --disable-asciidoc, see bug #1820.

comment:16 Changed 9 years ago by nickm

Ah, one more use case you need to think about. We want it to be the case that if the distributor has asciidoc they can "make dist", and then the user who gets the tarball can make and install the manpages and documentation without actually having asciidoc themselves. If I'm reading this right, we lose that ability here. True?

comment:17 Changed 9 years ago by nickm

Or I could be wrong. Maybe USE_ASCIIDOC is only false when the user has explicitly disabled asciidoc and the scenario I mentioned will still work. Hrmnmnm.

comment:18 Changed 9 years ago by ln5

Status: needs_reviewassigned

USE_ASCIIDOC should be false only on explicit demand by the user.

I've updated the changes/ file to reflect what's actually done.

I think build-wo-asciidoc-on-bsd is ready for merge.

comment:19 Changed 9 years ago by nickm

Resolution: fixed
Status: assignedclosed

Squashed and merged. Thanks again!

comment:20 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:21 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.