Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6524 closed enhancement (implemented)

[PATCH] move to non-recursive make

Reported by: stewart Owned by: stewart
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-client
Cc: ioerror, arma, jim@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This patch switches the automake foo over to non-recursive make.

This gives us a few benefits:
1) make -j clean all

this will start working, as it should. It currently doesn't.

2) increased parallel build

recursive make will max out at number of files in a directory,
non-recursive make doesn't have such a limitation

3) Removal of duplicate information in make files,

less error prone


I've also slightly updated how we call AM_INIT_AUTOMAKE, as the way
that was used was not only deprecated but will be *removed* in the next
major automake release (1.13).... so probably best that we can continue
to bulid tor without requiring old automake.
(see http://www.gnu.org/software/automake/manual/html_node/Public-Macros.html )


For more reasons why, see resources such as:
http://miller.emu.id.au/pmiller/books/rmch/

Technically, this applies on top of the patches in https://trac.torproject.org/projects/tor/ticket/6522 - but they're probably not *required*

Child Tickets

Attachments (12)

04-non-recursive-make.patch (33.0 KB) - added by stewart 7 years ago.
0001-Move-to-non-recursive-make.patch (35.3 KB) - added by stewart 7 years ago.
0002-add-subdir-objects-to-AUTOMAKE_OPTIONS-this-builds-o.patch (710 bytes) - added by stewart 7 years ago.
0003-remove-contrib-Makefile-and-contrib-suse-Makefile-fr.patch (814 bytes) - added by stewart 7 years ago.
0004-fix-dependencies-for-some-generated-files.patch (2.2 KB) - added by stewart 7 years ago.
0005-fix-TESTS-to-include-full-path-to-src-test-test.patch (583 bytes) - added by stewart 7 years ago.
0006-fix-circular-dependency-for-generating-code-digests.patch (713 bytes) - added by stewart 7 years ago.
0007-Fix-up-make-distcheck-and-greatly-simplify-docs-depe.patch (4.4 KB) - added by stewart 7 years ago.
0008-fix-up-calling-of-config.status-to-generate-docs.patch (770 bytes) - added by stewart 7 years ago.
tor-non-rec.diff (1.3 KB) - added by meyering 7 years ago.
use nodist_man1_MANS and s/AM_CONFIG_HEADER/AC_CONFIG_HEADERS/
tor-out-of-tree-fix.diff (3.4 KB) - added by meyering 7 years ago.
build: minimal adjustments to make out-of-tree build work
build-cleanup.diff (1.9 KB) - added by meyering 7 years ago.
make more/better use of automake's $(AM_V_GEN)/$(AM_V_at) variables

Download all attachments as: .zip

Change History (43)

Changed 7 years ago by stewart

Attachment: 04-non-recursive-make.patch added

Changed 7 years ago by stewart

comment:1 Changed 7 years ago by stewart

The most recent 2 patches are produced with "git format-patch", while 04-non-recursive-make.patch is not (so please ignore it and use the latter 2)

comment:2 Changed 7 years ago by nickm

Hm. This could be to review, since the diff renames the files *and* changes their contents. Next time, it would be easier to do renames and changes separate commits, so I can look at just the changes. No need to change it this time though.

I tried applying the first version patch to master, and it complained about .deps files when I did make; it looks like the transition isn't 100% clean, and people will need to be told to do a "git clean -xf; autogen.sh" before they proceed. That's ok.

Why did the src_test_*.o files get made with long_prefixes while others did't?

Then I tried the second two versions of the patches, and when I did "sh autogen.sh", I got:

[1134]$ sh autogen.sh 
autoreconf: Entering directory `.'
autoreconf: configure.in: not using Gettext
autoreconf: running: aclocal --force 
autoreconf: configure.in: tracing
autoreconf: configure.in: not using Libtool
autoreconf: running: /usr/bin/autoconf --force
autoreconf: running: /usr/bin/autoheader --force
autoreconf: running: automake --add-missing --copy --force-missing
configure.in:1318: required file `contrib/Makefile.in' not found
configure.in:1318: required file `contrib/suse/Makefile.in' not found
autoreconf: automake failed with exit status: 1

comment:3 Changed 7 years ago by nickm

Removing contrib/Makefile and contrib/suse/Makefile from configure.in fixed that last error. Now I get:

src/common/util_codedigest.c: In function ‘libor_get_digests’:
src/common/util_codedigest.c:10:25: fatal error: common_sha1.i: No such file or directory
compilation terminated.

...

src/or/config_codedigest.c: In function ‘tor_get_digests’:
src/or/config_codedigest.c:10:21: fatal error: or_sha1.i: No such file or directory
compilation terminated.

comment:4 Changed 7 years ago by nickm

Also, it looks like if we take this, we should add .dirstamp to .gitignore.

comment:5 Changed 7 years ago by stewart

these should now be fixed with patches 0003 through 0006. I haven't checked if "git clean -xf" is still required though, nor have I resolved why we get some longer object names in src/test. It should at least build now though :)

(I'd curse automake, but having spent enough time with the alternatives, I love it too much).

comment:6 Changed 7 years ago by stewart

latest news... I need to fix something for make distcheck, so please await my next patch :)

comment:7 Changed 7 years ago by stewart

This last two patches should fix up the last bit of make distcheck problems, and should be ready to go.

comment:8 Changed 7 years ago by nickm

0003 changes some .o to .c; seems wrong. ah. 0004 and 0006 change them back. Okay.

In 0007, why change nodist_man_MANS to man_MANS? The idea was that the tor.1 file (and its allies) would be generated from tor.1.in, and that we would distribute tor.1.in. With this change, it seems we're distributing tor.1, which should immediately get rebuilt. What's going on with that?

comment:9 in reply to:  8 Changed 7 years ago by stewart

Replying to nickm:

0003 changes some .o to .c; seems wrong. ah. 0004 and 0006 change them back. Okay.

In 0007, why change nodist_man_MANS to man_MANS? The idea was that the tor.1 file (and its allies) would be generated from tor.1.in, and that we would distribute tor.1.in. With this change, it seems we're distributing tor.1, which should immediately get rebuilt. What's going on with that?

This could be a bug... I had thought that man_MANS would ensure that the

man pages got into the make install target... although they do end up
there in the nodist_man_MANS... so I think I've gotten this one wrong.

comment:10 Changed 7 years ago by nickm

ok. I just tried replacing man_MANS with nodist_man_MANS again, and got an error from make distcheck:

make[2]: * No rule to make target doc/tor.1.txt', needed by doc/tor.1'. Stop.

Any suggestions there?

comment:11 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final

comment:12 Changed 7 years ago by nickm

(Remember to close #6665 if/when we merge this, and compare the relevant bit of #6665's patch with patch 0001 here)

comment:13 Changed 7 years ago by nickm

Status: newneeds_revision

Changed 7 years ago by meyering

Attachment: tor-non-rec.diff added

use nodist_man1_MANS and s/AM_CONFIG_HEADER/AC_CONFIG_HEADERS/

comment:14 Changed 7 years ago by meyering

I tried this series and found that AM_CONFIG_HEADER should not be used.
Also, it's recommended to use man1_MANS in your case (rather than man_MANS),
and hence nodist_man1_MANS, since you don't want to distribute them.

Patch for both attached:

comment:15 Changed 7 years ago by meyering

Cc: jim@… added

comment:16 Changed 7 years ago by meyering

To be clearer, AM_CONFIG_HEADER is obsolete.
Using automake from git/master, it has already been removed,
and this use provokes an error.

comment:17 Changed 7 years ago by nickm

Thanks, meyering! Do you think you might be able to do anything about the man_MANS/nodist_man_MANS problem discussed above? That's what I'm blocking on right now for merging this branch.

comment:18 Changed 7 years ago by meyering

nickm, after my patch that does s/man_MANS/nodist_man1_MANS/,
I did not see the problem you reported, and "make distcheck"
succeeded. I was using very recent automake and autoconf.
What version of those tools are you using?
I was doing a srcdir==. build, but distcheck also verifies
a non-srcdir (aka VPATH) build, so that should be fine.

If you give me enough info to reproduce the problem,
I'm sure I can resolve it.

comment:19 Changed 7 years ago by nickm

I've put all of these in a branch called "bug6524" in my public repository; there are links to my public repository at https://gitweb.torproject.org/nickm/tor.git . I split up your last commit into two before adding it.

I'm using Fedora 17; my autotools packages are up-to-date. (I have autoconf 2.68 and automake 1.11.3).

I checked out that branch, then did "git clean -xf" to make sure I didn't have any bogons lying around. Then I tried "sh autogen.sh; ./configure; make".

I got a build log that finally failed with:

/usr/bin/mkdir -p doc
  GEN    doc/tor.html
  GEN    doc/tor.html
config.status: error: cannot find input file: `doc/tor.html.in'
make[1]: *** [doc/tor.html] Error 1
make[1]: Leaving directory `/home/nickm/src/tor'

and I noticed that there were no build rules to actually say that doc/tor.html depended on doc/tor.html.in . So I tried adding this line to doc/include.am :

$(doc_DATA) : $(html_in)

And so far it seems to work. Running more tests now. Does the above addition look okay to you?

comment:20 Changed 7 years ago by nickm

I've tried a few more changes to quiet this down, on branch "bug6524_nm" in my public repository. So far, so good.

We must also test this with older autoconfs and automakes. I think our working target is automake-1.7 ; I don't recall our minimum autoconf.

comment:21 Changed 7 years ago by nickm

It appears there's a problem with automake-1.7. When I try to build with it, I eventually get:

if gcc -DHAVE_CONFIG_H -I. -I. -I.  -DSHARE_DATADIR="\"/usr/local/share\"" -DLOCALSTATEDIR="\"/usr/local/var\"" -DBINDIR="\"/usr/local/bin\"" -I./src/common     -g -O2 -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector -fwrapv -fPIE -Wall -fno-strict-aliasing -MT src/common/util_codedigest.o -MD -MP -MF ".deps/src/common/util_codedigest.Tpo" \
  -c -o src/common/util_codedigest.o `test -f 'src/common/util_codedigest.c' || echo './'`src/common/util_codedigest.c; \
then mv -f ".deps/src/common/util_codedigest.Tpo" ".deps/src/common/util_codedigest.Po"; \
else rm -f ".deps/src/common/util_codedigest.Tpo"; exit 1; \
fi
src/common/util_codedigest.c:12:1: fatal error: opening dependency file .deps/src/common/util_codedigest.Tpo: No such file or directory
compilation terminated.

comment:22 in reply to:  19 Changed 7 years ago by meyering

Replying to nickm:
...

I checked out that branch, then did "git clean -xf"...

That is the part I had not done.
With that, I see the same failure you do.
...

and I noticed that there were no build rules to actually say that doc/tor.html depended on doc/tor.html.in . So I tried adding this line to doc/include.am :

$(doc_DATA) : $(html_in)

And so far it seems to work. Running more tests now.
Does the above addition look okay to you?

That looks perfect.

comment:23 in reply to:  21 ; Changed 7 years ago by meyering

Replying to nickm:

It appears there's a problem with automake-1.7. When I try to build with it, I eventually get:

...

src/common/util_codedigest.c:12:1: fatal error: opening dependency file .deps/src/common/util_codedigest.Tpo: No such file or directory
compilation terminated.
}}}

IME, there is no point in accommodating such an old version of automake.
In fact, there are good reasons *not* to accommodate use of old
automake and autoconf, not least of which is security of those
who run "make dist" and "make distcheck". For reference, these
two bugs usually render those users vulnerable:

http://bugzilla.redhat.com/CVE-2012-3386
http://bugzilla.redhat.com/CVE-2009-4029

Add to that the fact that your (developer) time is precious
enough as it is. Don't waste it on something like that.
If anyone complains, tell them their use of old automake
poses a security risk and besides, also results in inferior
configure files.

That is why I put a line like this in configure.ac of each project I tend:

AM_INIT_AUTOMAKE([1.11.1 no-dist-gzip dist-xz color-tests parallel-tests])

That requires at least automake-1.11.1.
I should bump it to 1.12 soon.

Your users might appreciate it if you were to add "dist-xz" to that
list of automake options, since the xz-compressed tarball is 30% smaller
than the .gz one. For coreutils, grep and a few others, I've been
distributing only xz-compressed tarballs (i.e., dropped .tar.gz altogether)
for almost a year with no real complaints.

comment:24 in reply to:  23 Changed 7 years ago by nickm

Replying to meyering:

Replying to nickm:

It appears there's a problem with automake-1.7. When I try to build with it, I eventually get:

...

src/common/util_codedigest.c:12:1: fatal error: opening dependency file .deps/src/common/util_codedigest.Tpo: No such file or directory
compilation terminated.
}}}

IME, there is no point in accommodating such an old version of automake.

RHEL and Centos are are our main reasons for working with old automakes. It looks like we could bump up to 1.9, though, and not break any supported version. Next task: see if this works with automake 1.9.

comment:25 Changed 7 years ago by nickm

Took me a little while to try automake-1.9 , since my desktop doesn't seem to have one. But my FreeBSD VM does, and it seemed to work okay. So I bumped the requirement to 1.9. Now I'm looking into an issue with out-of-tree builds starting with a clean checkout.

comment:26 Changed 7 years ago by nickm

To reproduce the issue with out-of-tree builds, get to a clean checkout with "git clean -xf", then run autogen.sh, then try an out-of-tree build. It fails because there are some generated files that are placed out-of-tree when they aren't found, and which then aren't found by the compiler.

Changed 7 years ago by meyering

Attachment: tor-out-of-tree-fix.diff added

build: minimal adjustments to make out-of-tree build work

comment:27 in reply to:  26 Changed 7 years ago by meyering

Replying to nickm:

To reproduce the issue with out-of-tree builds...

Thanks for the description. The patch above fixed it for me.

Changed 7 years ago by meyering

Attachment: build-cleanup.diff added

make more/better use of automake's $(AM_V_GEN)/$(AM_V_at) variables

comment:28 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

Looking much better. I think this is mergeable.

comment:29 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Wrote a changes file and merged it.

comment:30 Changed 7 years ago by nickm

Keywords: tor-client added

comment:31 Changed 7 years ago by nickm

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