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

Unbump the syntect dependency #896

Closed
dtolnay opened this issue Mar 30, 2020 · 5 comments · Fixed by #898
Closed

Unbump the syntect dependency #896

dtolnay opened this issue Mar 30, 2020 · 5 comments · Fixed by #898
Labels
bat-as-a-library Related to bat-as-a-library bug Something isn't working question Further information is requested

Comments

@dtolnay
Copy link
Contributor

dtolnay commented Mar 30, 2020

I am interested in using bat in https://github.com/dtolnay/cargo-expand, but I don't want to use syntect 3.2.1 or 3.3.0. Those versions pull in onig 5.0, while syntect 3.2.0 uses onig 4.0 which has a more reliable cross platform build story.

Bat's syntect dependency is currently specified as 3.3.0:

bat/Cargo.toml

Lines 37 to 38 in 9a050cd

[dependencies.syntect]
version = "3.3.0"

It looks like this requirement was bumped from 3.2.0 in #628 and #671 for no particular reason. Could I request that the requirement in Cargo.toml be reverted to 3.2.0? Bat works with any of those versions and Cargo's resolver will give your users 3.3.0, but this allows cargo-expand to pin syntect 3.2.0 and still use bat.

Separately, consider disabling these dependabot bumps or find out if there is a way to bump Cargo.lock but not Cargo.toml. Bumping the Cargo.toml is not so bad when you are a binary but needlessly aggressive for libraries and causes unnecessary conflicts like us trying to build with syntect 3.2.0.


Longer term, if bat upgrades to syntect 4.0 that would also work for me because that version provides a way to disable the onig dependency. If you are upgrading to 4.0, please consider doing it in a way that allows downstream code to opt out of depending on onig:

[dependencies]
syntect = { version = "4.0", default-features = false }

[features]
default = ["syntect/default-onig"]
@dtolnay dtolnay added the bug Something isn't working label Mar 30, 2020
@sharkdp
Copy link
Owner

sharkdp commented Mar 30, 2020

It looks like this requirement was bumped from 3.2.0 in #628 and #671 for no particular reason.

Not quite, please see #650 (comment)

And the update to 3.3.0 fixes the bug described in trishume/syntect#259, which is related to #644.

Could I request that the requirement in Cargo.toml be reverted to 3.2.0? Bat works with any of those versions

I guess it compiles, but there are some bugs (e.g. it will panic on JSON files). So reverting to 3.2.0 is not that easy without also rolling back some syntaxes.

Separately, consider disabling these dependabot bumps or find out if there is a way to bump Cargo.lock but not Cargo.toml. Bumping the Cargo.toml is not so bad when you are a binary but needlessly aggressive for libraries and causes unnecessary conflicts like us trying to build with syntect 3.2.0.

Thank you for the explanation. I had not considered that. I guess we can leave the dependabot bumps active (although I have limited them to once a month recently), but we should be more careful about merging them and should consider skipping some updates.

Longer term, if bat upgrades to syntect 4.0 that would also work for me because that version provides a way to disable the onig dependency.

Long term: yes. Short term: probably not (see #650 (comment), trishume/syntect#285, and trishume/syntect#287).

@sharkdp sharkdp added bat-as-a-library Related to bat-as-a-library question Further information is requested labels Mar 30, 2020
@dtolnay
Copy link
Contributor Author

dtolnay commented Mar 30, 2020

Not quite, please see #650 (comment)

We've had the same problems in cargo-expand, since we target the same platforms. The most reliable fix has been pinning syntect "=3.2.0" and onig "=4.3.3". Trying to use 3.3.0 fixes some platforms but breaks others, as you can tell by #650 still being open.

I guess it compiles, but there are some bugs (e.g. it will panic on JSON files). So reverting to 3.2.0 is not that easy without also rolling back some syntaxes.

This is where as I've pointed out the right choices for an application vs a library are not the same. For bat as an application, what you wrote applies. For bat as a library it does not -- cargo-expand is only formatting Rust code, not JSON. We would like to make version choices that work the best for our use case. For bat as an application you can continue using 3.3.0 in Cargo.lock, but you should specify 3.2.0 in Cargo.toml (which means >=3.2.0 <4.0.0) so that downstream can use the right version in that range for their use case.

@sharkdp
Copy link
Owner

sharkdp commented Mar 30, 2020

Not quite, please see #650 (comment)

We've had the same problems in cargo-expand, since we target the same platforms. The most reliable fix has been pinning syntect "=3.2.0" and onig "=4.3.3". Trying to use 3.3.0 fixes some platforms but breaks others, as you can tell by #650 still being open.

Thank you for the explanation.

This is where as I've pointed out the right choices for an application vs a library are not the same. For bat as an application, what you wrote applies. For bat as a library it does not -- cargo-expand is only formatting Rust code, not JSON. We would like to make version choices that work the best for our use case.

Understood, I agree.

For bat as an application you can continue using 3.3.0 in Cargo.lock, but you should specify 3.2.0 in Cargo.toml (which means >=3.2.0 <4.0.0) so that downstream can use the right version in that range for their use case.

That sounds sensible. Are users guaranteed to get 3.3.0 if they use bat as an application, in this case? (I have my doubts since #881 was opened)

@dtolnay
Copy link
Contributor Author

dtolnay commented Mar 30, 2020

That sounds sensible. Are users guaranteed to get 3.3.0 if they use bat as an application, in this case?

If --locked, they are guaranteed to get what is in the lockfile. If not --locked, they get the most recent compatible version. Expanding the allowed range from >=3.3.0 <4.0.0 to >=3.2.0 <4.0.0 would never change what is the most recent compatible version.

@sharkdp
Copy link
Owner

sharkdp commented May 25, 2020

bat v0.15.2 can now be used with onig without the bindgen dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bat-as-a-library Related to bat-as-a-library bug Something isn't working question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants