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

Stabilize only part of the crate #154

Open
Kixunil opened this issue Feb 19, 2025 · 14 comments · May be fixed by #162
Open

Stabilize only part of the crate #154

Kixunil opened this issue Feb 19, 2025 · 14 comments · May be fixed by #162
Labels
1.0 Issues and PRs required or helping to stabilize the API

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 19, 2025

For a while I felt uneasy about stabilizing the whole crate. It takes from our time to stabilize more urgent things like units and primitives and the only types we know for sure we need are the errors. Up until this point I have tolerated it thinking it would resolve easily. However I remembered I wanted DisplayHex to have GAT.

Our options are:

  • Wait until Debian stable releases which will have GAT-capable compiler
  • Bump MSRV against our policy
  • Accept ugly API
  • Only stabilize parts that are directly required to have stable error types

I find the first three options highly annoying, which leaves us with the last one which also has its other advantages - relieving pressure to work on hex too much.

I suggest moving everything except decoding to hex-conservative-unstable crate, which will reexport everything such that it'd become a drop-in replacement for the hex-conservative crate (people will usually only need to change name in Cargo.toml and it should work for most code). Then we can stabilize hex-conservative pretty much immediately (but practically, I'd wait until we actually have something that depends on it close to stabilization). Then shift our focus on units and primitives.

@Kixunil Kixunil added the 1.0 Issues and PRs required or helping to stabilize the API label Feb 19, 2025
@apoelstra
Copy link
Member

Let's accept the ugly API. Adding another crate and another indefinite delay because of hypothetical features sounds awful.

If you want to use GATs we can release a 2.0, and we can semver-trick 1.1 to use the error types from 2.0, which will be functionally the same as what you're proposing, without adding a bunch of complicated indefinitely-open loops to all of our workloads.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 19, 2025

indefinite delay

We could set a deadline to the Debian release or some offset after (1mo?)

can semver-trick 1.1 to use the error types from 2.0

Oh, maybe yes! It needs to impl the old trait in terms of the new one but I think it's doable. I'd definitely want to try it out before committing into that approach though.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 19, 2025

Or a different possibility: have DisplayHex gated on compiler version. Those who need lower MSRV can just use the current hex-conservative for displaying. Though bitcoin would be one such crate (intentionally having duplicate dependencies) until the next Debian releases.

@apoelstra
Copy link
Member

Let me stew on this. It's an interesting general approach to "1.0" stuff that need a higher MSRV than our actual MSRV on 1.0-day. Or conversely, a way to artificially lower our MSRV by removing things that aren't doable with old rustcs.

In this particular case I'm a little skeptical, but I don't know the details of what you want to do with DisplayHex or why.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 19, 2025

Basically, the error types are our blocker and it's better to have a trait with fn foo(&self) -> Self::Type<'_> than do some other hacks. The specific problem with our reference hack is that foo(self) is confusing - looks like it consumes self but it shouldn't. And if we want to avoid even more confusion we need an additional IsRef trait to prevent accidentally implementing the trait on weird things. GAT is much easier to understand than this hack and it's just around the corner. IIRC it was stabilized in 1.65, Debian has 1.63. It's really annoying that 1.65 didn't end up in Bookworm but it's too late to complain about it. I've seen some conversations planning to get 2024 edition into the next Debian stable which would be really great.

BTW one other place we need GAT is Encode trait for push bytes (it needs type Encoder). I don't expect that thing to be stable before Debian release so I'm at ease there and I suspect this trick won't be applicable for other reasons.

@tcharding
Copy link
Member

At first when I read this I nearly cry'ed but after sitting with it all day its actually not a bad idea. I did #161 to demo it. Note that the changes to add constructors to all the error types will need thinking out better. I just hacked it up real quick to see how it works.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 21, 2025

I think if we simply make the unstable crate reexport the stable one then we can simply switch the dependency to be on the unstable crate and it should continue working.

@apoelstra
Copy link
Member

I think this might be exactly the same as the semver trick. Can't we do exactly this but with "the stable crate" being hex-conservative 1.0 and "the unstable crate" being hex-conservative 0.4?

@apoelstra
Copy link
Member

It's not quite the semver trick -- it's a sorta inverted version of it. But basically:

  • We release 1.0 with nothing but error types. It's basically unusable by itself, but ok.
  • We release 0.4 which re-exports the 1.0 error types and provides unstable version of everything else.
  • As we stabilize stuff, we move them into 1.1, 1.2, etc., and release 0.5, 0.6, etc which replace their unstable stuff with rexports from 1.x.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 21, 2025

I think this might be exactly the same as the semver trick. Can't we do exactly this but with "the stable crate" being hex-conservative 1.0 and "the unstable crate" being hex-conservative 0.4?

Yes! I wanted to suggest it too but held off for a reason I don't remember. I was probably thinking that you'll both prefer not to do two crates thing.

It's even better than semver trick because since we're going to break the API anyway and thus make a breaking SemVer bump, the weird edge cases around semver trick are not possible.

One thing I'm not sure about is how will we develop it in a single repository.

@apoelstra
Copy link
Member

We should have a separate branch for 1.0. When we eventually fold the 0.x series into the 1.0 branch we might need to do some illicit git-fu (e.g. synthetically creating a merge commit between 1.0 and master which has no diff when compared against 1.0) but that's fine.

When PR'ing into 1.0 those PRs will then look different (at least when we notice the target branch) which is good because we need to be more careful. In fact we could make Github enforce a 2-ack policy on exactly those commits.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 21, 2025

I meant using patch and stuff but I have now remembered that this is not the bitcoin repository, so we can just use the published crate.

@apoelstra
Copy link
Member

Yeah, doing this in bitcoin will be harder, though I suspect that if we have one branch for "all the 1.0s" and we actually publish them on crates as we modify them rather than using [patch], it could work. (Publishing vs [patch] should be okay since by definition we're not making breaking changes in the 1.x series.)

But here we don't even need to think about that much.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 21, 2025

bitcoin -> hex 0.x -> hex 1.0 (crates) should work I think regardless of whether we will use patch to get the leftmost dep. And we shouldn't need patch for 1.0 stuff at all.

@tcharding tcharding linked a pull request Feb 27, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Issues and PRs required or helping to stabilize the API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants