Fix feature semver violations in v4.7.0 #403
Conversation
It was not enough to make the `default` feature depend on it. Putting code behind a feature is in practice a semver breakage. See sharkdp/bat#1991 (comment) and trishume#402. It was added in trishume#345 after doing some refactoring. We can keep the refactoring, but we choose to remove the feature for now. Another option would be to make the feature opt-out rather than opt-in and rename it to `no-plist-load` or similar. That would be too unergonomic and confusing however, so for now we just remove it. We might add it back for v5.0.0.
When the `-rs` versions of dump creation and loading were introduced in syntect, `flate2` defaulted to a C implementation and made the Rust backend optional. Since two years, the Rust backend has been the default backend. See rust-lang/flate2-rs@c479d064e2. So we can stop using the `-rs` versions since enabling them makes no difference. Note that there is no mechanism yet that we can use to mark features as deprecated. See rust-lang/cargo#7130
|
I can't think of any changes to this PR, and I have verified locally that this fixes the build error in both So I think we should go ahead and merge this and release v4.7.1. Worst case we have to yank v4.7.1 too and try again in v4.7.2. A couple of words on the embarrassing mistake of forgetting the
The correct long-term fix to the |
|
I released v4.7.1! |
|
@trishume We need to yank v4.7.1 too 😢 While the code indeed compiles, it doesn't run, since the dump format has changed due to the lazy-loading changes: I am a very perplexed as to why I did not detect this problem when I verified my change. I must have accidentally and out of habit updated bat assets locally when verifying, and in that case everything works. It never occurred to me that we could have a problem with binary incompatibilities of dumps, but it is obvious in retrospect. I am terribly sorry to waste everyone's time again. I feel very bad about it. Right now I see no other option than having to bump major version. But let's not rush that release either to give some time to let the situation sink in and allow us think this through. Again. Since it appears necessary to bump major version, I think we should take the opportunity to at least do the following before doing a 5.0.0 release, which should be pretty simple:
|
|
To be clear: Projects using pure syntect and the assets that come with syntect should be fine. The problem is projects using bat, since bat ships its own versions of integrated syntaxes that now are binary-incompatible with the latest syntect release. |
|
Yup seems like a major version bump is necessary, I didn't think about the fact that changing the binary format requires a semver bump either. I sent you an offer to be a crates.io maintainer, you can feel free to yank releases in the future, and to publish releases on your own if I'm non-responsive for let's say 4 months, although I don't expect that I would be completely outside of extreme scenarios. |
Fixes #402
I ended up removing the
plist-loadfeature because I see no way of keeping it around in a semver compatible and sane way. The refactoring that made the introduction of that feature possible can we kept however, which is nice. So it should be easy to re-introduce it e.g. v5.0.0 at some later point.I did consider making
plist-loadopt-out rather than opt-in to remain semver compatible. We could call itno-plist-load, and if enabled, the unneeded code could be excluded from the build. I think that is too unergonomic and counter-intuitive however, so I think it's better to simply remove it for now.I also noticed that
dump-create-rsanddump-createare the same since 2 years, since 2 years ago the default flate2 backend was changed to Rust. See rust-lang/flate2-rs@c479d064e2. So there is no need to make a distinction any longer. This allows us to remain semver compatible with regards to the lazy-loading changes since nowparsingcan depend ondump-createanddump-loaddirectly. I did not bother adding an entry about that in CHANGELOG.md since there is no semantic change for clients.I could reproduce the build error in bat locally, and have confirmed that these changes fixes the build error.
I think it would be a good idea to not rush this PR in. Let's let it sit here for at least a few days so we have time to think this through and perhaps test some more. Any help with testing would be greatly appreciated.
(Oh, and I also noticed that I forgot to add a
pubto an API, so I fixed this too in this PR.)