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

[doc] developer-notes.md: point out that UniValue deviates from upstream #14882

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
8 participants
@Sjors
Copy link
Member

commented Dec 6, 2018

While debugging an issue I was somewhat surprised to learn that we've moved src/univalue from https://github.com/jgarzik/univalue to https://github.com/bitcoin-core/univalue, that these repos are both maintained and they're different.

The first mention of using the bitcoin-core repo is from late 2015 in #7157. I didn't check when the last common ancestor commit is.

I couldn't find documentation as to why (these things just happen in open source of course), but at minimum we should make this more clear.

There's also the following line in config.ac that I'm not sure what to do with:

AC_INIT([univalue], [1.0.3],
        [http://github.com/jgarzik/univalue/])
@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@fanquake fanquake added the Docs label Dec 6, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Upstream univalue wasn't (or hardly) maintained for a long time so we've at some point started maintaining it ourselves.

Updating the various URLs makes sense.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

utACK a67d713

@jnewbery

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

ACK a67d713. Documentation should reflect reality.

Reviewers: please also consider reviewing a clarification to the README for our univalue fork: bitcoin-core/univalue#17

@MarcoFalke MarcoFalke merged commit a67d713 into bitcoin:master Dec 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Dec 6, 2018

Merge #14882: [doc] developer-notes.md: point out that UniValue devia…
…tes from upstream

a67d713 [doc] developer-notes.md: point out that UniValue deviates from upstream (Sjors Provoost)

Pull request description:

  While debugging an issue I was somewhat surprised to [learn](#14164 (comment)) that we've moved `src/univalue` from https://github.com/jgarzik/univalue to https://github.com/bitcoin-core/univalue, that these repos are both maintained and they're different.

  The first mention of using the bitcoin-core repo is from late 2015 in #7157. I didn't check when the last common ancestor commit is.

  I couldn't find documentation as to why (these things just happen in open source of course), but at minimum we should make this more clear.

  There's also the following line in `config.ac` that I'm not sure what to do with:
  ```
  AC_INIT([univalue], [1.0.3],
          [http://github.com/jgarzik/univalue/])
  ```

Tree-SHA512: e58105677b5ebe0005772282da4a805fee7dfccacfb1b2686a874517bf46072d1481181f8a8865d25526f6ed9e5fcd55d8d49906bf27cd0f5aefe4f258aa4d63
@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

The autoconf thing is still different, right?

@Sjors Sjors deleted the Sjors:2018/12/doc-univalue branch Dec 6, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@karel-3d I didn't touch that, as I have no idea what that line is doing.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Upstream univalue is still maintained. I'm not sure why @laanwj is claiming otherwise.

That we have a fork of it at all, is a bug.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

@luke-jr it might be worth a separate Github issue to discuss if we want to migrate back to upstream.

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Upstream univalue is still maintained. I'm not sure why @laanwj is claiming otherwise.

It has very spottily maintained for a long time, and at the time we needed some fixes in. I haven't checked the status for a long time so it may be different now. I'm not "claiming otherwise" I think I was very precise in my wording.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@Sjors I opened #15009

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.