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

Bitcoin Core should not have a fork of univalue #15009

Closed
luke-jr opened this issue Dec 20, 2018 · 6 comments
Closed

Bitcoin Core should not have a fork of univalue #15009

luke-jr opened this issue Dec 20, 2018 · 6 comments

Comments

@luke-jr
Copy link
Member

luke-jr commented Dec 20, 2018

There is no justification for forking UniValue. The subtree should be restored to the official upstream and the fork deleted.

@jnewbery
Copy link
Contributor

This is a complete waste of time. You've NACK'ed a bunch of useful PRs (bitcoin-core/univalue-subtree#11 (comment), #14164 (review), etc) because you believe the upstream should be jgarzik/univalue without once giving a justification of why that should be.

jgarzik/univalue appears to be very infrequently maintained. There haven't been any code changes since August, and a request to add a 1.0.4 release (prompted by you here: #12666) has been unacknowledged since August 21st.

Switching to having our own repo was a pragmatic decision to avoid relying on a spottily-maintained single-maintainer repo, as Wladimir rightly points out here: #14882 (comment).

jgarzik does not appear to be interested in actively maintaining his repo. Why should we add that dependency and demand that he maintain it for us?

@luke-jr
Copy link
Member Author

luke-jr commented Dec 20, 2018

Not-forking should be the default; only forking should require justification.

It hasn't needed any code changes since August AFAICT.

Despite the stale open issue, v1.0.4 was released Sep 1: https://github.com/jgarzik/univalue/releases/tag/v1.0.4

cc @jgarzik

@maflcko
Copy link
Member

maflcko commented Dec 20, 2018

Note that the repo is missing a test fix, that we needed: jgarzik/univalue#57 (created pull request per next comment)

Also our copy has the deprecated pair wrappers removed to avoid introducing them in future code changes and to avoid having to constantly fix them up again. I think I explained that earlier. Are we running in circles here?

@luke-jr
Copy link
Member Author

luke-jr commented Dec 20, 2018

Where is the test fix PR upstream? That is an example of why we shouldn't have a fork...

@jgarzik
Copy link
Contributor

jgarzik commented Dec 21, 2018

  • univalue is actively maintained upstream (jgarzik/univalue)
  • To the specific issue of deprecated pair wrappers, upstream has had - for a long time now - a method of compiling those out, to provide a certainty that future Bitcoin code cannot use them.

@maflcko
Copy link
Member

maflcko commented May 8, 2020

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

@maflcko maflcko closed this as completed May 8, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants
@jgarzik @fanquake @jnewbery @luke-jr @maflcko and others