Unsubtree Univalue #25369
Unsubtree Univalue #25369
Conversation
|
Concept ACK. But given how far we've diverged from upstream univalue, and upstream library hardly being maintained, this already has been effectively the case for a while. This just removes some extra steps. |
|
Concept ACK. Apart from simplifying our workflow, this also clarifies that the univalue lib in its current form is unmaintained and any effort to try supporting it as a system library is a largely wasted effort. I can see the reason for using a system library for json stuff, but ideally it should be something that is maintained. Looks like there is https://github.com/cculianu/univalue which could serve as a drop-in, but I am not sure how much it is maintained, how much it is available as a system package, how much effort it would be to switch to it and if it is even possible to use as a drop-in replacement, considering their and our past changes. There is also https://github.com/nlohmann/json, which is not a library, but a single header file. So I am actually thinking it wouldn't be too much hassle to make our univalue a single C++17 header file as well. At which point it could be trivially copied by other projects (or trivially distributed as a "library"), if needed. Though, that seems best to be discussed in a separate thread (or maybe not at all, since it is out-of-scope?). |
|
If we only used |
5548ef1 to
46e0aaf
Compare
Don't forget that before univalue, we used another library (JSON Spirit). And the switch to Univalue was made specifically because it was designed for a purpose that most JSON libraries (at the time, at least) didn't provide: exact roundtripping of the string representation of values (so that it would be possible to use custom encodings and parsings of floating-point values, bypassing the Concept ACK. The reality is that the current univalue codebase we're using is effectively maintained by us already, and we're relying on many improvements that the original upstream doesn't have. I'd be open to discussing alternatives, as I agree an externally maintained library is certainly better if a suitable candidate exists, but that seems like an independent discussion from this PR. |
|
I've fixed the CI ( |
I sure remember, I still remember how much of a pain it was to change over to univalue, we had all kinds of bugs for many releases. I just don't think it's a good use of time to do that now again. Do we have any issues with univalue? |
Sorry, I didn't mean to argue in favor of moving away from univalue; quite the opposite. It works fine for our purposes (as it was specifically designed even to cover one of our rather unusual requirements). The only downside is that it requires us to maintain it going forward, but if we're ok with that - no concern. The only point I wanted to make is that the discussion about whether or not to de-subtree univalue is independent from the discussion about moving to another library altogether. |
|
Concept ACK. We effectively maintain this anyway, so we may as well maintain it conveniently. I think we'll want to get away from Univalue eventually, but I agree with @sipa, that's not what this is about. Modularity is unaffected. It exports the same interface as before. |
|
Concept ACK. There is a trade-off between being modular and maintenance burden. The most extreme example of that trade-off in action is the Node Package Manager ecosystem. We too could split off individual classes in If there was a more convenient way to have individual git repos for tightly integrated small components like that, that would be nice. Then we could revisit what to do with UniValue. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Remove all the files we don't use. This should not change behavior, or anything at all, as none of these files are currently used in our build system.
46e0aaf to
c673e1d
Compare
c673e1d to
b7918b8
Compare
There was a problem hiding this comment.
ACK c673e1d 🔬
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK c673e1dbad8f532052827f05045695c8717c32b2 🔬
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjpeQv/Tnz8e79UCsuOokHrmC/5gPv1bQQ86m+5CDXPCZJDx2C6iG+H593QsHSZ
QTW4Z2lKTTtBdMp+bUY739MrmiIBBcx2naSyBeoBY1Sh4e/C+5C1MZ3ZvYpIVj0T
Oqv99WX5IbbjyYpPueyzbo49MEF5yem6lpmK3POxn0rst6FOh3GeSVJ3OezDZmHR
ECWSnGXa+SLUdwYwQ4AHsC0hEs/75X0sIqqz/9QKBtdyXzfdDXak0KURfXxbwE0b
jEePPiDwrpOLyEbKcpy9qQyCrk9E+mILmOv9O49wHhNo4X1j1YV+jFLNDuZHmPqk
9JQhOOR7RUF82HEZ/c5E//mZM9Ijm+1zVx0tYqWOXS2ql+1J/lsYBDpRUj/KfHIo
gCE4bAorwPAhLI1VOfFFJoRE99tuXtTKgyUXTDXFLRtKWq4pAkNQOmGXgZqSk/Zz
1poBqvkDK6IREspH2dCD9mWj0QrRj8yMFCWT4WrWxTYVfQ0oMRG6VJ2aP4hX22JT
0idJ4kyQ
=qAvf
-----END PGP SIGNATURE-----
b7918b8 to
e0954e8
Compare
Mostly changes to remove src/univalue exceptions from the various linters, and the required code changes to make them happy. As well as minor doc changes.
e0954e8 to
d873ff9
Compare
|
Code review ACK d873ff9 |
|
re-ACK d873ff9 only changes: 📼
Show signatureSignature: |
At this point, maintaining Univalue as a subtree doesn’t serve much purpose, other than being an inconvenience for making changes to the code (along with polluting our repo with a number of files we don’t use). Our Univalue fork currently deviates from the upstream API, and for some time has been marked as not-maintained for use by other projects (I'm not aware of any that use it). The upstream Univalue is not maintained, and has not been for some time. There are no new releases, bugs remain unfixed, and PR's we've upstreamed, https://github.com/jgarzik/univalue/pulls, are not being commented on/merged.
Another substantial benefit of no-longer maintaining a subtree is removing the rather awkward work-flow currently required to make changes to the Univalue code, particularly breaking changes / introducing new features, e.g. bitcoin-core/univalue-subtree#27. We need to dance around and merge changes to our fork, with a flag, then pull them down here, then switch to using the new code, then go back to our Univalue repo, and remove the old code / flag, then pull the repo down here again, and remove our usage of the flag. Quite the overcomplicated mess.
With this PR I'm proposing we stop treating Univalue like a subtree, or upstream project/fork, and going forward, treat it as part of this codebase, which we can refactor directly (with pulls to this repo. Ideally, after this is merged, our univalue subtree repo could be marked as "archived". In this repo, I think there is a good chance that the Univalue code will ultimately be refactored away into "modern" C++, i.e using
std::variant(at least one person has played around with doing this).Univalue history:
--system-univalueoption introduced: Build against system UniValue when available #7349Suggestion was to use system Univalue by default.
This was pushed back on by contributors, as well as the upstream Univalue maintainer (jgarzik).
It is not maintained for usage by other projects. Notably, the API may break in non-backward-compatible ways.: [docs] Update readme bitcoin-core/univalue-subtree#17the API is broken in non-backward-compatible ways.: doc: note that our API has diverged from upstream bitcoin-core/univalue-subtree#30--system-univalueoption removed: build: tighter Univalue integration, remove--with-system-univalue#22646Guix Build (x86_64):
Guix Build (arm64):