Autotools/vpath support #11
Conversation
- added macro in main configure.ac for running configure script for leveldb - added autogen script to launch autoreconf in leveldb - added the most basic configure.ac for leveldb - added references to srcdir instead of hardcoded path
|
Concept ACK |
|
I don't think that the generated configure script should be included in the pull request. Maybe update the .gitignore? |
.gitignore
Outdated
| autom4te.cache/ | ||
| config.log | ||
| config.status | ||
| !configure |
There was a problem hiding this comment.
Agree that a generated script like configure shouldn't be in the repo. A typo, I think.
There was a problem hiding this comment.
The pull request lists it as a feature, so I assume it's intentional.
|
The addition of the configure script is intentional. The rationale is consistency with secp256k1 library which includes a configure shell script. Without the configure script, parent projects would need to run autoconf or autoreconf on leveldb. |
The problem with committing build-generated scripts is that when developers do run (as a convenience, ./configure script is part of the tarball created with Also I suppose the only intended parent project here is bitcoin, which will handle calling the autoconf. |
|
Tested ACK apart from the configure nit - see bitcoin/bitcoin#7466 (comment) |
|
There is no ./configure script included in the secp256k1 repository, by the way: https://github.com/bitcoin/secp256k1 nor https://github.com/bitcoin/bitcoin/tree/master/src/secp256k1 |
Agreed! Let me fix this so that there is no configure script and that gitignore is properly aligned to ignore the output of autoreconf. |
…ed .gitignore to ignore this file.
2. Added a slash in Makefile input so as to not modify the main Makefile when configure is run. The result is a double slash when referring to paths in the build, but this is harmless.
|
@theuni can you take a look here please? |
|
Hmm, I wonder if it would be simpler to just add our own simple Makefile.am here, rather than trying to pile on their stuff. I'll whip something up for comparison. |
|
Well this is already done, and checked it works. |
|
Pushed an alternative to theuni/bitcoin@3b71469 The benefit is that it's directly integrated into our build without the need for another configure run. I'm not married to it, but I'm not a fan of adding more complexity to what should be a pretty straightforward build. |
|
I agree, it's a worthwhile alternative - however, please read the discussion in bitcoin/bitcoin#7466 on how we got to this solution. I think the advantage of your solution is that it can be maintained entirely outside of leveldb's tree (given that the .include is moved). Effectively it absorbs leveldb into bitcoin's build system. Not wedded to either though, I just want out of tree builds to work. |
|
@theuni I've tested your patch, but it fails on Seems every executable now needs both |
|
@laanwj I've pushed a cleaned up version here: https://github.com/theuni/bitcoin/tree/leveldb-integration. Having done that cleanly now, I do prefer it over the wrapper approach. That's not to diminish @kleetus's work, of course. If you're ok with the approach, I'll PR it here. |
|
@theuni Looks good to me! You probably want to pr it at bitcoin/bitcoin though, not here. |
|
Right, I forgot the file was moved up. PRing over there. |
|
Closing this in favor of the build system changes in Bitcoin Core. It may still make sense to try to do this upstream. |
…bitcoin-core#5. 8b1cd37 fixup define checks. Cleans up some oopses from bitcoin-core#5. (Cory Fields) Pull request description: - use "#if defined(foo)" rather than "#if foo" - Use the same guard for the cpuid header and the function Tree-SHA512: fe83895055faf9f5491b9af44262a4dc15d9f56ec8f818e7d66c1002bb6568a90345662828abc7baab0772baa646f9cf13f8ba586ebad5fc3678731b27585885
5 participants
closes #10
Discussion
LevelDB currently has a hand-rolled Makefile and it seems to work well, especially for build systems that do not have a shell (Windows). What the build system for LevelDB does not have is support for Autotools, specifically leveraging autconf, autoreconf, and the macros that come along with that.
LevelDB is, fundamentally, a project that should be a dependency to another project with its own build system. Many of these projects are also open-source and use Autotools. As such, those projects can easily integrate this project if, and only if, this project has at least a configure.ac file. Otherwise those projects need to hack in running "make -c" in their own Makefile.am's.
This code adds