Lazy implementation of semver trick for 0.15 #209
Comments
|
Hmm That's cool, An example is rust-bitcoinconsensus at version |
|
Ah, good catch! Didn't notice there are essentially hidden public items. So I guess we would have to reimplement them? Or is there any workaround for it? |
|
Ah, I see now where's the problem, it's just that prefixes changed. Forwarding those symbols somehow should work, I think. |
|
You can't fix it without causing the same linking issues you're trying to avoid(multiple definitions of the same symbol) EDIT: It seems like it isn't possible to define weak symbols in stable rust rust-lang/rust#29603 |
|
Shouldn't be an issue because the library will be included only once. Or is there any other instance of a different library defining those same symbols? |
I think I might've misunderstood your original problem, are you trying to avoid linking twice to the C library because of binary sizes, or are there actual linking errors? (and rust style errors or C linking errors?) |
|
|
The previous version of the crate also contained exported symbols as a part of invisible, but used public API. At least bitcoinconsensus was using these symbols. This change reexports them under the old names, so they can still be used. See rust-bitcoin#209
|
I managed to fix it! 🎉 The latest commit fixes Would be nice if someone looked at it to see if the change is sound (no UB), but at least the tests passed. |
|
I really like this in principle but am having trouble deciding whether this would constitute a breaking change (and if it did, whether it's one so edge-casey or work-around-able that it's worth improving ergonomics for old versions of the library). |
|
Worst case we could just revert all the changes, release a new version and tell all projects that have double-linking problems to search for another solution, right? |
|
Heh, yes, in principle we can always "revert if somebody complains" but if we break things for downstream users, that still causes them pain for a day or two, and if they don't think/don't want to complain, maybe we won't ever fix it. |
|
I guess the other question is what are the two dependencies that depend on stale libsecps? I presume one of them is RL, which hopefully we can fix soon, but is that enough to resolve the issue a different way (especially given this shouldn’t be an issue again, it only is for past versions?).
… On Apr 18, 2020, at 09:01, Andrew Poelstra ***@***.***> wrote:
Heh, yes, in principle we can always "revert if somebody complains" but if we break things for downstream users, that still causes them pain for a day or two, and if they don't think/don't want to complain, maybe we won't ever fix it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Yeah, would be better to find out who exactly uses it and how, to know if the trick works reliably enough. Wouldn't want to recklessly break stuff. Just if we do everything we can to avoid breakage without stagnating and it breaks anyway, we have a way out. Rust lightning is the stale dependency that causes There's also the grin version, but looks like it doesn't conflict for some reason. IDK how it's possible. If the decision ends up "don't release semver trick version", then maybe mentioning that it exists and is usable via |
|
Hmm, went to read the docs to see if they promise anything about C symbols being available for other libraries. There's no such thing, so I'd say that the users shouldn't depend on them just because it works. Feels like bypassing the usual stability conventions. Of course, if symbols are something that is supposed to be available and stable, then it should be documented. Documenting that bitcoinconsensus is a special blessed library would be fine too IMO. |
|
FYI, noticed that I broke MSRV, but I fixed it easily. |
I'm a little confused by this. It seems like the only way to hit this issue would be to have two versions of rust-secp in the dependency tree that both predate the switch to versioned symbols. Or is the other such dependency libbitcoinconsensus? |
|
Ah, yes, the symbols were renamed, I forgot. This is the error with unpatched version: So apparently the symbols from the old version conflict with zkp (grin) version.
Pretty much what's happening here, it's just that one of them is essentially renamed. What's really strange is how come that my other commit to fix bitcoinconsensus didn't break it again?? I went to double check it and made sure to run Cargo tree before semver trick |
That's what I was talking about. |
|
@Kixunil thanks for doing this. FYI, I'm working on updating The problem is that adoption of new hash type system rust-bitcoin/rust-bitcoin#349 has introduced a lot of breaking changes in the code; it takes a lot of time to hunt all of them down. After this change updates must be easier and rust-lightning probably will always match the latest rust-bitcoin release. |
|
@elichai yes, I understand now what you were talking about, however, that macro does not break @dr-orlovsky yeah, I noticed. Bumping the version was the first thing I attempted to see if it's trivial enough. When I saw the errors, doing the semver trick seemed easier. 😆 |
|
Just did version bump for electrs project BTW; have taken about 3 hrs to complete: romanz/electrs#238 Lightning looks more complex though... |
|
@dr-orlovsky I've listened to the devcall and while the newer versions have versioned symbols, the issue is that not everything is updated to the newer version yet. Of course, upgrading is a cleaner solution, lazy semver trick is just a workaround to get something working quickly. |
|
Yes, seems like the only way is to fork and update to the newer version manually... I will try it in the next few days |
|
Might be worth asking people from grin to use the same trick with renaming symbols. |
Feel free, they're best on a very old version of this repo, I hope this PR will nudge them into a rebase/merge, and maybe they'll even adapt this renaming symbols trick FYI, if you're linking both you can enable the feature |
|
I tried external symbols and it didn't work for me. |
Can you please expand on that? |
|
Sorry, I don't remember what the error was exactly. I tried to set that feature and compile, but got some other errors. I might give it another try in a few days, if it's still relevant. |
|
Hmm sorry I forgot we were talking about 0.15. |
|
As for rust-lightning. Given we merged the version bump this morning, does that resolve the issue here, at least for the immediate needs of those in this thread (when using rust-lightning master)? |
|
lnp-node now builds fine, closing. |
Hello, I'm unable to create a PR against non-existing branch, so I'm opening this as an issue.
I managed to implement version
0.15.6in terms of usingsecp256k1-sys. This is very useful because it avoids linking the C library twice, creating link errors. Such errors prevented me from buildinglnp-node. It compiles fine when my change is used (tested viapatchinCargo.toml). CC @dr-orlovsky, you probably want to know about this. :)All tests (except fuzztarget) for
secp256k1are passing of course. I guessfuzztargetis something special used for fuzzing only, so it doesn't matter that it's not passing, right?It'd be nice if you created a new branch (maybe without the
-lazy-semver-tricksuffix) in your repo with my changes and published the0.15.6crate to crates.io. I created two commits expecting that it'll make code review easier, but feel free to squash it, if you want. (I wrote the commit message to be easy to modify. :))See the description in Kixunil@10a33cc to understand what I mean by "lazy" :)
I hope you will find this helpful!
The text was updated successfully, but these errors were encountered: