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

Semver violation in v4.7.0 and v4.7.1 #402

Closed
Emoun opened this issue Dec 26, 2021 · 8 comments · Fixed by #403
Closed

Semver violation in v4.7.0 and v4.7.1 #402

Emoun opened this issue Dec 26, 2021 · 8 comments · Fixed by #403

Comments

@Emoun
Copy link

Emoun commented Dec 26, 2021

The 4.7.0 release violates semver. See:

@Emoun Emoun changed the title Server violation in v4.7.0 Semver violation in v4.7.0 Dec 26, 2021
@trishume
Copy link
Owner

Ah crud, I didn't notice the moving APIs behind a feature thing. I can yank the release and publish again as 5.0.0

@trishume
Copy link
Owner

I yanked 4.7.0 from crates.io

@trishume
Copy link
Owner

I'm going to take a look at the features to see if I can rejigger them to be semver-compatible, but otherwise I'll just publish this release as 5.0.0 with migration notes for features.

@Enselic
Copy link
Collaborator

Enselic commented Dec 26, 2021

@trishume I'm so sorry to cause troubles for you 🙁I'm not at home at the moment. If I were, I would work on this with highest prio.

Since v4.7.0 is yanked there is no urgency to make a new release I think? In a few days I'll have time to try I come up with a solution. One option on the table is to revert the change so we don't have to do a major bump. But hopefully I can come up with a solution.

@trishume
Copy link
Owner

@Enselic yup no rush, and don't worry about it, it wasn't out for long and I did look over those changes and not notice the implications of the features added either. I may or may not have time to look at things myself, we'll see who gets there first.

Another potential option is to first release a minor version with compatible features but hopefully load time improvements, then do a major version bump so that people on the old version still get the improvements but maybe not some other things.

Enselic pushed a commit to Enselic/syntect that referenced this issue Dec 28, 2021
It is 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
Enselic pushed a commit to Enselic/syntect that referenced this issue Dec 28, 2021
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.
@Enselic
Copy link
Collaborator

Enselic commented Dec 28, 2021

This should be fixed by #403.

I could reproduce the plist-load related build error locally, and can confirm that the PR fixes the build in that case.

It also fixes a semver problem related to the parsing feature that has not been reported yet, but that I can confirm existed. Most easily triggered by doing cargo build --no-default-features --features minimal-application in bat.

@Enselic
Copy link
Collaborator

Enselic commented Jan 3, 2022

I am terribly sorry, but we need to yank v4.7.1 and reopen this issue. See #403 (comment).

So so sorry about this :(

@Enselic Enselic reopened this Jan 3, 2022
@Enselic Enselic changed the title Semver violation in v4.7.0 Semver violation in v4.7.0 and v4.7.1 Jan 3, 2022
@Enselic
Copy link
Collaborator

Enselic commented Jan 6, 2022

v4.7.1 has also been yanked now, so the urgent problem this issue is about has been taken care of. Closing.

@Enselic Enselic closed this as completed Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants