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

Replace univalue subtree with proper dependency on external UniValue #7340

Closed
wants to merge 6 commits into from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Jan 14, 2016

This is not consensus-critical code, so really had no excuse to be subtree'd in the first place.

@luke-jr luke-jr force-pushed the luke-jr:sys_univalue branch 2 times, most recently to 1d13699 Jan 14, 2016
@luke-jr
Copy link
Member Author

luke-jr commented Jan 14, 2016

Actually, this loses a bugfix, so @jgarzik needs to release UniValue 1.0.2 to make this sane...

@luke-jr
Copy link
Member Author

luke-jr commented Jan 14, 2016

Also, I believe the Travis failure crash is unrelated to this PR, and a bug in UniValue (or Core) that we need to fix independently of this - but I can't reproduce it... :/

@luke-jr luke-jr force-pushed the luke-jr:sys_univalue branch to 4752e33 Jan 14, 2016
@@ -0,0 +1,21 @@
package=univalue
$(package)_version=1.0.1
$(package)_download_path=https://codeload.github.com/jgarzik/$(package)/tar.gz

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jan 14, 2016

Member

I think we should use the bitcoin/univalue.git repository to allow multiple people to maintain the repository (the subtree is also pointing to bitcoin/univalue.git).

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 14, 2016

Author Member

AFAIK that's just a mirror of @jgarzik 's upstream? Note he could very well add additional maintainers to his upstream repo as well, if he wants.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jan 14, 2016

Hmm.. not sure about that.
If I'm right, this would mean, in order to compile bitcoin-core, users need to checkout the univalue git repository and compile univalue by themselves unless there is broad support over package managers (apt-get, etc.)?

IMO we should use subtrees for dependencies that are not available over common package managers (bdb4.8 might be an exception because a) not really necessary [--with-incompatible-bdb] and b) wallet only).

@laanwj
Copy link
Member

laanwj commented Jan 14, 2016

NACK. Univalue is a nice and small library, and it's useful to have it included.. This makes it much easier for people that want to build.

I'd be OK with the option to use an external univalue, but don't remove the internal one.

@luke-jr
Copy link
Member Author

luke-jr commented Jan 14, 2016

@laanwj Is it really so much harder to run:

make -C depends univalue
@MarcoFalke
Copy link
Member

MarcoFalke commented Jan 14, 2016

make -C depends univalue

You'd have to update doc/build*

luke-jr added 4 commits Jan 15, 2016
Static linking libstdc++ (as is done by Travis and gitian) with shared libraries breaks empty std::string with GNU
@luke-jr
Copy link
Member Author

luke-jr commented Jan 15, 2016

Apparently the depends system can't actually be used in the way I expected.

While I still think this is the correct way to go, I'll start with making it an option for now... in #7349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.