Create build-aux/m4 dir. Move m4 scripts for out-of-src builds. Tidy away pkg.m4 script. Readibility and fixes AC_CONFIG macro calls in configure.ac. Add warnings for autoreconf.
Move m4 to standard location. Beginning of out-of-src build work but useful on its own. Tested on linux RH-derivative, ubuntu and OSX 10.7. Needs further cross-platform testing.
Isn't the problem with out-of-source directory builds that leveldb can't cope with them?
Yes leveldb will be a large part. Here are some preliminaries that are standard practice structure anyway.
I'd love to see VPATH builds supported, but we'll have to decide if we want to carry changes to the leveldb autofoo.
pkg.m4 at root is deliberate, it allows for installs that don't have pkg-config macros.
When running autoconf, pkg.m4 is pulled from the system if present from the path "pkg.m4". If it's not present, this file is substituted. If the file is in any other path, the substitution won't happen. So it either needs to stay there, or we must only support autoconf versions where it's built-in. That would mean just deleting this file.
This is necessary for at least osx 10.6, not sure where else.
build-aux is where auto* installs all of its files as necessary. src/m4 are source files under our control. I don't see any compelling reason to combine them?
As @laanwj said, leveldb is what stands in the way of out-of-tree builds. We would need to take on the burden of enumerating their files. Personally, I'd be fine with that since we pull from upstream quite rarely.
"pkg.m4 at root is deliberate, it allows for installs that don't have pkg-config macros. When running autoconf, pkg.m4 is pulled from the system if present from the path "pkg.m4". If it's not present, this file is substituted. If the file is in any other path, the substitution won't happen. So it either needs to stay there, or we must only support autoconf versions where it's built-in. That would mean just deleting this file. This is necessary for at least osx 10.6, not sure where else."
This not the observed behaviour. pkg.m4 is picked up by aclocal not autoconf. On linux usually the system pkg.m4 is installed in /usr/share/aclocal/pkg.m4. autogen.sh runs autoreconf which is a combination of aclocal automake autoconf libtoolize
./configure runs after autogen.sh so the current set-up includes the bitcoin supplied pkg.m4 preferentially ahead of system pkg.m4, which is what I assumed you wanted and thus my mod does not change that behaviour, merely tidies files into one known location for m4 scripts.
The OSX 10.6 problem is more likely due to Fink or Ports installing pkg-config and brew installing autotools, or vice versa, in which case pkg.m4 is not being found, i.e. user should check /opt/local/share/aclocal/pkg.m4 or /sw/share/aclocal/pkg.m4 or /usr/local/share/aclocal/pkg.m4 or /usr/share/aclocal/pkg.m4
I suggest remove pkg.m4 and allow system pkg.m4 to be used as default in the majority of cases.
@theuni Would it be possible to patch the leveldb build system to support out-of-tree build? I'd prefer that to enumerating the files ourselves, because it could be upstreamed. I agree that taking up the list of leveldb files is not an extreme extra maintenance burden, but all these small things do add up.
It'd be easy enough, but I don't think they'd be interested in accepting it. The problem is that autotools forces you to list each file rather than globbing, which is a really really good thing. That prevents stray files from slipping into release tarballs.
To fix in an upstreamable way, we'd have to split their makefile up into 2 files, one which just lists sources, and the other to handle build logic. We would include the first from one of our Makefiles. From their POV, there would be no compelling reason to accept that.
More realistically, we'd just enumerate those files ourselves. Anything left out would not be copied to our release tarballs, so builds would fail. Since we update leveldb very rarely anyway, I think it'd be a reasonable burden to take on.
@randy-waterhouse Yes sorry, i meant aclocal. And after some testing, it seems you're correct about the include order for pkg.m4 as well. The wording here led me to believe that the system m4 would have precedence, but an 'aclocal --verbose' indeed shows our file being used first.
That being said, I'm not willing to break build on some platforms just for some "neatness". Surely we can use an ifndef on one of the pkg-config macros to determine if we need to user our own file or not?
@theuni have you actually verified that an aclocal/pkg.m4 really is non-existent on those platforms or is it the user's misconfigured pkg-config/autotools set-up not passing correct include paths? The code right now looks like a hack in the bitcoin build code that is inadvertently affecting behaviour on all systems to load a non-system pkg.m4. Keeping things neat and in known locations makes debugging simpler and provides integrity for path dependent functionality generally ... basically it's good practice.
I'm not sure that we have to cater to a allegedly broken build environment on an ancient MacOSX version. Do we know anyone actually building with that? Or that can test?
@laanwj It for sure broke my 10.6 environment before I moved to a new 10.9 macbook. Generally, I'm opposed to knowingly breaking something for no known benefit. That being said...
I've been working on retooling the osx environment. It's looking like Apple is moving to make 10.9 the new minimum dev requirement as quickly as possible, and I want to be sure we're ready for that. I've got the cross and native build up and running with recent clang and 10.9 sdk (still 10.6 minimum runtime, so no user-facing changes other than better functionality on newer OSs).
Point being: when we're looking to bump up our minimum required dev environment, I have no problem dropping these kind of compat workarounds. Without good reason though, I just don't see why we would.
#4765 I moved the less contentious fixes to a separate PR ...
Re: the existing 'workaround' ... that it is bypassing correct functionality on all other platforms should be a bigger consideration than a nearly obsolete platform?
The OSX 10.6 'problem' has not been verified and from studying the documentation (and experience) is most likely due to a misconfigured system than a true lack of pkg.m4 script (which has been bundled with pkg-config installs since at least 2006).
The problem it fixes is to correct the behaviour of the autoconf system to use a local platform pkg.m4 script and not the hacked provided one of bitcoin src. And the code is tidier.
The osx 'problem' has been verified by me. I personally added the fix because it is needed on my 10.6 macbook. A quick googling turns up lots of hits like this one: https://stackoverflow.com/questions/8578181/using-the-pkg-config-macro-pkg-check-modules-failing
Again, if you're going to argue that it's reasonable to remove some functionality that someone may be relying on that's perfectly fine and I could most definitely be persuaded. But you still haven't given a tangible benefit other than "the code is tidier". I get that using the system m4 is the correct thing to do. I really do. But if there's no tangible benefit, then that point carries no weight.
At this point I really don't care because this is a silly thing to be arguing over. Ultimately, I just really dislike the "maybe break a working thing for the sake of technical correctness" type of changes.
Use 'aclocal --print' to determine the directory in which to look for pkg.m4 then add that output to your include for autogen is the correct fix. aclocal -I /opt/local/share/aclocal
I think we can agree to disagree if you are unwilling to accept that the current code is not functioning correctly because of the botched "workaround" implementation. I agree that it is a silly little thing to be arguing over ...
Does this need to be rebased every day?
If you split off the removal of pkg.m4, which is the only controversial part, this could have been merged almost a month ago..
Update .gitignore.
@laanwj OK, I split off the custom pkg.m4 removal (will put in separate PR for that fix).
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4719_b3e53394263839989357ff1248ca13e797664c55/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
These auto-generated files need to be in .gitignore:
?? build-aux/compile
?? build-aux/test-driver
Merged via 6fc1dc1 (with .gitignore addition)