Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

msvc: build secp256k1 and leveldb locally #14372

Merged
merged 2 commits into from Jan 31, 2019

Conversation

Projects
None yet
9 participants
@ken2812221
Copy link
Member

commented Oct 2, 2018

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

src/leveldb/ should not be edited locally – the leveldb changes should probably be submitted upstream instead :-)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@ras0219-msft

This comment has been minimized.

Copy link

commented Oct 2, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Concept ACK on using the subtree when compiling with msvc. Though, could the subtree bump be done in a separate pull request?

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Some of the leveldb changes proposed in this PR are already in PR's on the leveldb repo, specifically bitcoin-core/leveldb#14 and bitcoin-core/leveldb#7 (as an aside the changes in bitcoin-core/leveldb#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.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

@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.

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch 2 times, most recently Oct 8, 2018

@DrahtBot DrahtBot removed the Needs rebase label Oct 8, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch Oct 8, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch Oct 21, 2018

@DrahtBot DrahtBot removed the Needs rebase label Oct 21, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch Nov 8, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 8, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch Nov 8, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Concept ACK, makes sense if MSVC follows the autoconf-based build system in this regard.

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch 3 times, most recently Jan 22, 2019

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch to 2140ac6 Jan 22, 2019

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Jan 26, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Needs rebase

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch from 2140ac6 to 4a2b193 Jan 31, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 31, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

@sipsorcery @NicolasDorier Is this good to merge?

@ken2812221 ken2812221 force-pushed the ken2812221:2018-10-02-msvc-code branch from 4a2b193 to 82dcacb Jan 31, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 31, 2019

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

tACK 5209106.

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)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 31, 2019

Merge bitcoin#14372: msvc: build secp256k1 and leveldb locally
82dcacb msvc: build leveldb locally (Chun Kuan Lee)
5209106 msvc: build secp256k1 locally (Chun Kuan Lee)

Pull request description:

  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

Tree-SHA512: aa2cc1c3191e8d9cab23d555da4be296314c46d944f452c2ec6202b1779e4cc223b603e589b38196cd2c793a03a8bb0ba128cc66256b35a58c5e7bb358475206

@MarcoFalke MarcoFalke merged commit 82dcacb into bitcoin:master Jan 31, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

post merge tACK.

ken2812221 added a commit to ken2812221/bitcoin that referenced this pull request Feb 1, 2019

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 4, 2019

Merge bitcoin#15325: msvc: Fix silent merge conflict between bitcoin#…
…13926 and bitcoin#14372

bef8fdd msvc: Fix silent merge conflict between bitcoin#13926 and bitcoin#14372 (ken2812221)

Pull request description:

  The bitcoin-wallet.exe would have to link with libsecp256k1 after we build libsecp256k1 in project.

Tree-SHA512: cb3fafa301f39121f5d26ac8ac6009c9665fcad1061dbf14ba013104870abe5413ac57c97c97df12b6ba2ad709b776c51aeec20d41f3ae01d3460a5e18f40eec

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

@ken2812221 ken2812221 deleted the ken2812221:2018-10-02-msvc-code branch Feb 9, 2019

ken2812221 added a commit to ken2812221/bitcoin that referenced this pull request Feb 14, 2019

MarcoFalke added a commit that referenced this pull request Feb 14, 2019

Merge #15407: msvc: Fix silent merge conflict between #13926 and #14372
… part II

3c6ef03 msvc: Fix silent merge conflict between #13926 and #14372 part II (Chun Kuan Lee)

Pull request description:

  In #15325, I added secp256k1 as a dependency of bitcoin-wallet. However, I didn't notice that leveldb is also a dependency of it.

Tree-SHA512: dc29b5cad6c529dd9517d6c2cbbe5297b69e73303e2fbbcd4b4842c9c5b51a4332df5a4bf3b82cd3ed2c1668cc95f8c9636f9485af0d722fed9c1319da3cc2e2

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.