In current MSVC build setup, the code depends on leveldb and secp256k1 that are installed from vcpkg which is not controlled by us. If we update our code, we have to wait for vcpkg port being merged.
This PR move them from vcpkg to local branch to make it as same as autoconf.
The leveldb changes is based on bitcoin-core/leveldb#14 and bitcoin-core/leveldb#18
src/leveldb/ should not be edited locally – the leveldb changes should probably be submitted upstream instead :-)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
If you want to maintain forks of leveldb/secp256k1 locally then it probably makes more sense to do this than install them via vcpkg.
However, if the intent is to treat them as third party libraries, you might be happier with instead replacing the vcpkg portfiles to point at the modified versions before running install (C:\tools\vcpkg\ports\leveldb\). We always use the local catalog to do the build, so they can just be directly replaced before calling vcpkg remove --outdated; vcpkg install.
Note: If you want them to be removed by remove --outdated, you need to make sure to change the version in ports\leveldb\CONTROL when making modifications.
Concept ACK on using the subtree when compiling with msvc. Though, could the subtree bump be done in a separate pull request?
Some of the leveldb changes proposed in this PR are already in PR's on the leveldb repo, specifically https://github.com/bitcoin-core/leveldb/pull/14 and https://github.com/bitcoin-core/leveldb/pull/7 (as an aside the changes in https://github.com/bitcoin-core/leveldb/pull/14 were the ones I included in the leveldb vcpkg portfiles).
For what it's worth I'd agree with @practicalswift that changes to the sub-trees should be done upstream.
I'm a concept ACK on using subtrees for leveldb and secp256k1 for the msvc/Visual Studio build. It does make debugging a little bit easier.
@sipsorcery Sorry. I'm not aware that you already submmitted a PR for that. I'll revert the duplicate thing in bitcoin-core/leveldb#18 @practicalswift @sipsorcery I know that they should be done upstream, but they are not done yet. I could show the result before then and make CI happy. Anyway, this PR shouldn't be merged before upstream one merged.
Concept ACK, makes sense if MSVC follows the autoconf-based build system in this regard.
Needs rebase
@sipsorcery @NicolasDorier Is this good to merge?
tACK 52091066be15a86a38c4db182338808f9316c35a.
Builds correctly on VS2017, VS2019 and AppVeyor.
(One note is that Pieter Wuille did comment somewhere that the libsecp256k1 config options set here meant that Windows ecdsa operations were up to 4 times slower than on Linux. I had a quick look but because msvc doesn't support asm on 64 bit builds didn't find any way to improve it)
post merge tACK.
Milestone
0.18.0