Opened 5 years ago

Closed 5 years ago

#15053 closed defect (implemented)

Improve out-of-tree builds

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: build 026-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Several make rules don't work when building out-of-tree. There are also some files that aren't cleaned up on make clean or make distclean. Lastly, some file generation blobs don't work properly by leaving temporary files behind.
These are just some of the things i have ran into so far and i am currently patching these issues.

Also creating this ticket to discuss certain parts of the build configuration that are unfamiliar and for which the reasons of implementation can't be traced back.

Child Tickets

Change History (24)

comment:1 Changed 5 years ago by cypherpunks

Is it necessary for the updateVersions script to be run on every configure?
The related code in configure.ac;

if test -x /usr/bin/perl && test -x ./scripts/maint/updateVersions.pl ; then
  ./scripts/maint/updateVersions.pl
fi

comment:2 Changed 5 years ago by nickm

Is it necessary for the updateVersions script to be run on every configure?

Hrm, maybe not. Is this the only issue here?

We *do* use out-of-tree builds as a part of our build process; make distcheck usually complains if there's undeleted stuff. What files are you seeing?

comment:3 Changed 5 years ago by nickm

Keywords: 026-backport added
Milestone: Tor: 0.2.7.x-final

Marking for 0.2.7; conceivably backportable for 0.2.6 if it's clean and important, but 0.2.7 is likelier.

comment:4 in reply to:  2 Changed 5 years ago by cypherpunks

Replying to nickm:

Is it necessary for the updateVersions script to be run on every configure?

Hrm, maybe not. Is this the only issue here?

We *do* use out-of-tree builds as a part of our build process; make distcheck usually complains if there's undeleted stuff. What files are you seeing?

This whole endeavor started with the command line argument test failing (removed now in #14806) because of a hash mismatch (maybe an argument for keeping --digests as discussed in #14742?). Turns out i did an in-tree compilation before doing make distclean and going out-of-tree. This left src/or/or_sha1.i in the source tree while also being created in the out-of-tree. However, the source tree has preference over out-of-tree during compilation resulting in the inclusion of the file with the old hashes.

Another file is micro-revision.tmp which is created when a make is executed more than once. This is caused by an error in the make rule for micro-revision.i. The temporary file is not removed with make distclean.

comment:5 Changed 5 years ago by cypherpunks

I'm currently working on fixing the coverage_html rule. I'd suggest to make this rule optionally available based on whether --enable-coverage has been given during build configuration. When coverage has not been enabled, the files needed by lcov aren't generated and it will fail.

Keeping user-friendliness in mind, we could also display a message when this rule is called without coverage enabled.

Changed 5 years ago by cypherpunks

Changed 5 years ago by cypherpunks

Changed 5 years ago by cypherpunks

comment:6 Changed 5 years ago by cypherpunks

And here are the patches. They are divided as follows;

  • 0001 makes the rules work from out-of-tree builds and cleans some additional files generated by the various rules.
  • 0002 fixes the issues discussed in comment:4.
  • 0003 removes all relative paths from header files for consistency (some header files use relative paths, some not). Relative paths are not needed because all the relevant directories are included in the search path already.
  • 0004 removes the line mentioned in comment:1.
  • 0005 implements the suggestion in comment:5.
  • 0006 speaks for itself.

There is one remaining issue with the check-docs rule because some paths are hard coded in checkOptionDocs.pl. I have a solution and will upload the patch asap.

comment:7 Changed 5 years ago by cypherpunks

Patch 0007 uses autotools to substitute *_srcdir and *_builddir. The resulting file is not executable so we need to call perl directly (after letting autotools find it for us).

comment:8 Changed 5 years ago by nickm

For 0002, I was worried about users who don't have the means to create those files, but then I saw that we actually create them empty if necessary. Should we be removing them from EXTRA_DIST though? It seems strange to have something both in CLEANFILES and EXTRA_DIST.

For 0004, maybe updateVersions.pl should have a "make update-versions" target corresponding to it.

comment:9 Changed 5 years ago by nickm

Status: newneeds_review

comment:10 in reply to:  8 Changed 5 years ago by cypherpunks

Replying to nickm:

For 0002, I was worried about users who don't have the means to create those files, but then I saw that we actually create them empty if necessary. Should we be removing them from EXTRA_DIST though? It seems strange to have something both in CLEANFILES and EXTRA_DIST.

I think we should remove them from EXTRA_DIST. There is no need to include them in the distribution if they are generated during compilation (and removed during make clean).

For 0004, maybe updateVersions.pl should have a "make update-versions" target corresponding to it.

Good idea.

comment:11 Changed 5 years ago by cypherpunks

FYI 0009 depends on 0007 because it defines the PERL variable.

comment:12 Changed 5 years ago by cypherpunks

Small fixup so the zero_length_keys test is called correctly. Ran into the error when testing make distcheck.

comment:13 Changed 5 years ago by nickm

feature15053 in my public repository has this branch, squashed, with some .gitignore changes on top of it. It passes "make distcheck", "make", and "make check", which is a good set of signs.

comment:14 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Still looks okay to me; merged. Thanks!

Note: See TracTickets for help on using tickets.