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

Find a replacement for rust-yaml #467

Open
mitsuhiko opened this issue Mar 26, 2024 · 9 comments
Open

Find a replacement for rust-yaml #467

mitsuhiko opened this issue Mar 26, 2024 · 9 comments

Comments

@mitsuhiko
Copy link
Owner

Today insta now has to vendor rust-yaml largely because there is no replacement available.

  • serde_yaml: deprecated
  • yaml-rust2: too many dependencies, too new MSRV
@torokati44
Copy link

too many dependencies, too new MSRV

I get your point, but also, this is relative / subjective. If the users of insta are okay with this, that's up to them.

Anyway, from an outside perspective, having a fixed-up version of the original rust-yaml vendored as source doesn't seem that bad either. It's a bit of potential bloat, but oh well...
It just adds to your maintenance burden, which is up to you to decide if it's too much, of course...

@mitsuhiko
Copy link
Owner Author

mitsuhiko commented Mar 26, 2024

I get your point, but also, this is relative / subjective. If the users of insta are okay with this, that's up to them.

Insta is a testing library. By definition it has to support the rust versions that people are targeting, so it needs to be significantly more conservative than other libraries. There are users of this crate which have very conservative MSRV targets. It might be that over time the solution is to be more deliberate about the MSRV support and to encourage people to use older versions of insta but that so far has not been the way this has been approached.

The dependencies themselves I'm very cautious about. Insta has a relatively low number of dependencies so that a) I can ensure a conservative MSRV and b) because every dependency causes pain. Case in point: yaml-rust today. yaml-rust2 is also unable to pass the minver check since it depends on encoding which itself is currently failing CI.

@mitsuhiko mitsuhiko mentioned this issue Mar 26, 2024
@Ethiraric
Copy link

Ethiraric commented Mar 26, 2024

Hi! I'm sorry that yaml-rust2 proves to not be an option for your YAML parsing needs.

I had not taken into account the MSRV when updating the library. What would be the MSRV you are currently targetting?

Also, are you using the encoding features of yaml-rust? If you were not using davvid's fork, then adding a feature flag for the encoding library should fix the MSRV of yaml-rust2.

Edit: This definitely did not come from the encoding change.

@jplatte
Copy link

jplatte commented Mar 26, 2024

Hey @mitsuhiko, just read your blog post. My experience as a user of your libraries has been stellar and from my POV you absolutely didn't need to immediately react to #463 and any duplicates or make the rustsec warning disappear for these folks.

On the flipside, I think it's good that there is now #467 that people can watch if they want to know about updates to this situation. I also think it's good that people got unmaintained crate warnings (whether it should cause CI to fail is a matter of taste IMHO), such that they can investigate, which in this case likely lead many insta users here and to #467 and to subsequently silencing this specific advisory (well actually since you now vendored the lib, I no longer have to, but was about to do that).

Anyways, thanks for being on top of things, and try not to let people stress you out :)

Thanks also @Ethiraric for putting a bunch of effort into a high-quality YAML crate for Rust, including the new potential considerations for lowering the MSRV!

@Ethiraric
Copy link

Ethiraric commented Mar 26, 2024

With the following changes, I can get MSRV down to 1.65.0:

  • Move debug stuff behind a feature flag (removes the need for the std::sync::OnceLock)
  • Remove PartialEq, Eq from YamlDecoderTrap (this technically is a breaking change, again...)

I can further get it down to 1.64.0:

  • Remove let...else statements

Going further down would require tracking down changes in allocator-api2 (transitive dependance of hashlink) or finding a replacement for hashlink.

@mitsuhiko
Copy link
Owner Author

@Ethiraric so the thing here is that insta is a pretty odd bird here and to which degree does it even make sense for a library like yaml-rust2 to cater to it. Insta also really values stable snapshots so having depended on a largely unmaintained library was pretty beautiful in that regard :)

When I looked at yaml-rust2 other than the MSRV I noticed that it now depends on encoding which fails in CI for me with the minver check (https://github.com/mitsuhiko/insta/actions/runs/8418550704/job/23093352977):

  error: failed to parse manifest at `/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/encoding-index-singlebyte-1.0.20140915/Cargo.toml`
  
  Caused by:
    library target names cannot contain hyphens: encoding-index-singlebyte

Also it depending on encoding makes my spidery senses tell me that I might be in a similar situation in no time, as that crate is also running dangerously close to unmaintained detection.

As for MSRV that is currently targeted: 1.51 is what it has today, but for the next major I'm considering pushing it up to 1.67 which is the lowest that can be supported by the other upgraded libraries.

@Ethiraric
Copy link

If I'm not mistaken:

  • If I hide the encoding dependency behind a feature flag (you shouldn't depend on it, since you were using yaml-rust and not davvid's fork)
  • And hide the debug stuff behind a feature flag

Then yaml-rust2 would satisfy your needs for your next major? No encoding and their dependency, and yaml-rust2's MSRV would then be 1.65.0.

I don't consider it particularly "catering" to your needs. Taken out of context, those 2 changes do make sense. The debug stuff was behind a #[cfg(debug_assertion)] anyway and only included in debug builds. I just changed that to `#[cfg(feature = "debug_prints")].

Encoding is something many wouldn't need. I can leave that as an on-by-default feature that you would manually have to disable. This removes dependencies, speeds up compile-time and decreases binary bloat for those who wouldn't use it.

I honestly think those 2 changes would do great to the crate.

On a totally unrelated note, I do plan on some changes for the library (discussed here). I hadn't anticipated serde_yaml to get archived as well, and I think having a "central" YAML set of crates might be good.

When are you planning on releasing your next major version?

@mitsuhiko
Copy link
Owner Author

When are you planning on releasing your next major version?

No huge urgency. There are a few things that accumulated alongside that are worth addressing in a major bump.

@Ethiraric
Copy link

Alright. I'll do the changes on yaml-rust2 and mention this issue in the PR. I'll make sure it contains the necessary documentation to have MSRV set to 1.65.0.

By the time you make the release, there's a high chance that we move development to another crate, notably to have a similarly-named set of crates for all YAML-related things.

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

No branches or pull requests

4 participants