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

Please consider a new release with updated dependencies #33

Closed
kinnison opened this issue Jul 9, 2024 · 34 comments
Closed

Please consider a new release with updated dependencies #33

kinnison opened this issue Jul 9, 2024 · 34 comments

Comments

@kinnison
Copy link

kinnison commented Jul 9, 2024

I use yaml-rust2 in a project where we're not ready to commit to something as potentially breaking as saphyr. We'd like to eventually since we primarily just use the content which would be in saphyr-parser but for now we're kinda sticking to more stable crates.

Unfortunately we're very size-sensitive and complexity-of-dependency-tree sensitive and as a result we could do with a version of yaml-rust2 which has its hashlink dependency updated to 0.9.

I am prepared to organise a branch and testing to ensure the update is good; though given hashlink's API is part of yours, it probably implies that we would need to do a 0.9.0 release of yaml-rust2 rather than 0.8.2.

Is this something that this project would entertain, given that saphyr-parser does not appear to be presenting as production-ready yet?

@Ethiraric
Copy link
Owner

I can use the following set of dependencies without changing any piece of code:

[dependencies]
arraydeque = "0.5"
encoding_rs = { version = "^0.8", optional = true }
hashlink = "0.9"

[dev-dependencies]
libtest-mimic = "0.4"
quickcheck = "1.0"

I took the opportunity to remove the patch version requirement, which may help with binary size.
Only the transitive dependency hashbrown would not be updated. The rest would be dev-dependencies.

libtest-mimic has had a bunch of updates, but updating it fires a ton of errors that I don't want to spend time fixing.
I think it probably is a bit too early for a 0.9.0? But I have no experience in evaluating that. I'm not fond of the idea of bumping the minor for just a dependency update.

Since I have not changed any code in the crate, I wonder if it's okay to just release a patch version?
It might break if someone is using a set hashlink dependency on their project alongside yaml-rust2 though.
Something like:

hashlink = "0.8"
yaml-rust2 = "0.8.newversion"
let map = hashlink /*v0.8*/ ::new(something);
let yaml = yaml_rust2::Yaml::Hash(map /*expects 0.9*/);

I'm open to advice and suggestions (cc @davvid)

@GuillaumeGomez
Copy link

You can't prevents dependencies to mess up with semver unless you pin each version you're depending on with =.

@Ethiraric
Copy link
Owner

Ethiraric commented Jul 9, 2024

I am not worried about yaml-rust2 having issues with one of its dependencies. It's more about not messing up consumers of yaml-rust2.

I have no idea how common practice it is to use a dependence and one of its dependence (in this instance someone using yaml-rust2 and hashlink) directly. I have had to do it recently and the experience was atrocious.

I wouldn't want to be "that guy" that messes up other's projects with a patch bump that should have been a minor bump.

@GuillaumeGomez
Copy link

There is cargo-semver-check which allows you to check semver breakages.

@Ethiraric
Copy link
Owner

@kinnison Are you planning to use yaml-rust2 to build marked-data?

There's an issue with saphyr where Marks are off. They sometimes do not return the correct line and/or column and/or index.
Since saphyr's code is based on yaml-rust2, I fear this may suffer the same issue.

Unfortunately, Marks were untested in yaml-rust and I have not written any test for them during development.

@kinnison
Copy link
Author

kinnison commented Jul 9, 2024

@kinnison Are you planning to use yaml-rust2 to build marked-data?

There's an issue with saphyr where Marks are off. They sometimes do not return the correct line and/or column and/or index.
Since saphyr's code is based on yaml-rust2, I fear this may suffer the same issue.

Unfortunately, Marks were untested in yaml-rust and I have not written any test for them during development.

Well that's worrying. Yes the dependency comes via marked-yaml though the size-sensitively project is within my employer. I've not noticed marks being particularly off except sometimes at the outsides of containers, but for the most part we care about the start location of scalars only.

I have some basic tests in marked yaml for marks but perhaps I should add some more.

@Ethiraric
Copy link
Owner

If you're able to contribute those to this crate directly, that would be great!

If you use the Parser API directly, you could write test similar to what we have in saphyr. Except the assert_eq! would contain the marks as well.

@Ethiraric
Copy link
Owner

Also, semver-checks doesn't seem to be concerned about the hashlink dependency:

$> cargo semver-checks
 Parsing yaml-rust2 v0.8.2 (current)
  Parsed [   1.170s] (current)
 Parsing yaml-rust2 v0.8.1 (baseline)
  Parsed [   2.054s] (baseline)
Checking yaml-rust2 v0.8.1 -> v0.8.2 (minor change)
 Checked [   0.007s] 69 checks; 69 passed, 6 unnecessary
Finished [   3.241s] yaml-rust2

@kinnison
Copy link
Author

FYI when I updated hashlink in marked-yaml I chose to do a "major" bump because it was a major bump in hashlink and the type is exposed in the API quite strongly. I'd almost suggest making the new version 0.9 in order to follow hashlink's version - it's fortuitous that it's aligned right now and that'd keep things simple for consumers of the library.

@kinnison
Copy link
Author

If you're able to contribute those to this crate directly, that would be great!

If you use the Parser API directly, you could write test similar to what we have in saphyr. Except the assert_eq! would contain the marks as well.

The tests inside marked-yaml probably will need faffing with quite hard to make them suitable for either this crate or for saphyr-parser though I'm happy to consider that part of my goal when I sit down to do this. I am very grateful for your (and others') efforts in these libraries and so I'll certainly do my best to gather the energy/effort to help if I can.

@davvid
Copy link
Collaborator

davvid commented Jul 10, 2024

I'm not opposed to bumping minors since we're still in 0.x and there's not really any practical downside to bumping up. IIRC semver technically allows breaking changes in patch releases for 0.x versions, but that would be naughty.

@Ethiraric
Copy link
Owner

Since for now you're the only one needing the hashlink update and you need markers, I think we can release once we have markers fixed? If that sounds reasonable to everyone.

@kinnison
Copy link
Author

Since for now you're the only one needing the hashlink update and you need markers, I think we can release once we have markers fixed? If that sounds reasonable to everyone.

I think that sounds fair. I'm unlikely to be able to get to it very very soon, but I'll note it at work as being part of what we need to do to minimise duplicated dependencies.

Thank you for being open to this.

@Ethiraric
Copy link
Owner

No worries! I want this project to be useful. Wouldn't be if I turned down opportunities to improve ;)

I'm nearing the end of a heavy saphyr-parser refactor to better optimize in-memory string handling.
If all goes well and I didn't kill performance too much on other benchmarks, I might merge that and get to fixing markers.

@Ethiraric
Copy link
Owner

Actually, I could specify the dependency as hashlink = "0.8, 0.9". What is exposed of the API does not seem to change much since no test break. This would enable users to use either version as they please, and use 0.8 or 0.9 as they need.

@kinnison
Copy link
Author

Actually, I could specify the dependency as hashlink = "0.8, 0.9". What is exposed of the API does not seem to change much since no test break. This would enable users to use either version as they please, and use 0.8 or 0.9 as they need.

That sounds very good. I wasn't even aware that was possible.

@Ethiraric
Copy link
Owner

I wasn't even aware that was possible.

That's because it isn't! I should have tested this. The , acts like a && and not a || making the above version requirement impossible to satisfy.

0.8, < 0.10 appears to work. It should mean "at least 0.8 but nothing equal or higher to 0.10".

@kinnison Can you check it works for you on the update_deps branch, commit 30a42ab?

@GuillaumeGomez
Copy link

You can actually. An example: GuillaumeGomez/sysinfo#1295

@Ethiraric
Copy link
Owner

Your solution seems similar to 0.8, < 0.10, if I'm not mistaken.

@GuillaumeGomez
Copy link

Indeed!

@kinnison
Copy link
Author

Sadly that doesn't seem to have fixed things:

indolence(git)(🦀🏠)% cargo tree -d 
hashlink v0.8.4
└── yaml-rust2 v0.8.1 (https://github.com/Ethiraric/yaml-rust2?rev=30a42ab#30a42abd)
    └── marked-yaml v0.7.1 (/home/dsilvers/dev-git/general-rust/marked-data/marked-yaml)

hashlink v0.9.1
└── marked-yaml v0.7.1 (/home/dsilvers/dev-git/general-rust/marked-data/marked-yaml)

@Ethiraric
Copy link
Owner

Have you by any chance tried to cargo clean in-between? And to remove the Cargo.lock? Also, what happens if you try to cargo update?

@kinnison
Copy link
Author

I tried cargo clean and cargo update - interestingly the latter tells me it is not updating hashlink:

indolence(git)(🦀🏠)% cargo update --verbose                                             
    Updating crates.io index
    Updating git repository `https://github.com/Ethiraric/yaml-rust2`
     Locking 0 packages to latest compatible versions
   Unchanged hashlink v0.8.4 (latest: v0.9.1)
note: to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`
indolence(git)(🦀🏠)%                                                                    

@kinnison
Copy link
Author

I am beyond my meagre understanding of Cargo'd dep management :(

@Ethiraric
Copy link
Owner

Same. I tried reading the book but it says the resolver should choose the most recent update matching the requirements, or one that is already imported if available.

I changed the requirement to >= 0.8, < 0.10 and it worked in a test repository. Can you try again?

@kinnison
Copy link
Author

Yep cargo update now updated hashlink

@Ethiraric
Copy link
Owner

I pushed that change to master. You can specify the commit as a dependency to your crate for now, if that's urgent. I'll postpone the update until I have fixed markers.

@kinnison
Copy link
Author

I pushed that change to master. You can specify the commit as a dependency to your crate for now, if that's urgent. I'll postpone the update until I have fixed markers.

Fantastic, thank you. I've unfortunately not been able to get into writing some tests for you yet though :( Sorry.

@dtolnay
Copy link

dtolnay commented Jul 14, 2024

Please do not put hashlink = ">= 0.8, < 0.10" in a release. This does not mean "yaml-rust2 works with both hashlink 0.8 and hashlink 0.9". It means "yaml-rust2 works with a random one of hashlink 0.8 or hashlink 0.9 that you can't control. But only one of them." It is never appropriate to use that for any dependency that appears in a crate's public API.

@Ethiraric
Copy link
Owner

I'm interested in knowing more about that.

What I thought the requirement expressed was "you can use any 0.8 or 0.9 you want". If hashlink follows SemVer requirements, this should not break anything, can it?
My understanding in reading Cargo's resolver documentation is that it attempts to select the highest version that match the requirements. In the aforementioned situation, it would pick the 0.9 that is specified alongside yaml-rust2. If one had specified hashlink = "0.8.1", I'd expect yaml-rust2 to use 0.8.1.

I don't understand the "only one of them" part. The crate has to be linked to a specific version of hashlink regardless. Do you mean we can't control in a scenario where crate A uses hashlink = 0.8, crate B uses hashlink = 0.9 and crate C depends on A, B and yaml-rust2? I would have expected yaml-rust2 to use that of crate B, which has the highest version.

To me, >= 0.8, < 0.10 appeared like a "best of both worlds" solution. It's pushed on master but not yet released (glad I postponed it).

What solution would you think would be best? Force 0.9?

@dtolnay
Copy link

dtolnay commented Jul 14, 2024

Do you mean we can't control in a scenario where crate A uses hashlink = 0.8, crate B uses hashlink = 0.9 and crate C depends on A, B and yaml-rust2? I would have expected yaml-rust2 to use that of crate B, which has the highest version.

And A will not compile, because yaml-rust2 does not work with hashlink 0.8. It works with a random one of 0.8 or 0.9 that A does not control.

Hashlink 0.8's LinkedHashMap and hashlink 0.9's LinkedHashMap are not the same type. Crate A cannot pass hashlink 0.8's LinkedHashMap to Yaml::Hash if that data structure only accepts hashlink 0.9's LinkedHashMap. It has nothing to do with whether hashlink uses semver.

@Ethiraric
Copy link
Owner

Hashlink 0.8's LinkedHashMap and hashlink 0.9's LinkedHashMap are not the same type. Crate A cannot pass hashlink 0.8's LinkedHashMap to Yaml::Hash if that data structure only accepts hashlink 0.9's LinkedHashMap.

This I knew because I have recently been faced by that issue.

And A will not compile, because yaml-rust2 does not work with hashlink 0.8. It works with a random one of 0.8 or 0.9 that A does not control.

I do not understand why A wouldn't compile here. A does not use yaml-rust2. I would think there would be 2 versions of hashlink in the binary. One 0.8 that would operate with A, and a 0.9 that would operate with B and yaml-rust2. Of course, if C takes a structure from A and passes it to yaml-rust2, I expect it to fail. But I expect the compile error to originate from C and not A


I don't see any situation here where setting a specific version requirement on yaml-rust2 would solve the issue. It may maybe make the issue more visible? That's all I can think of. If we set to:

  • 0.8: no interop between B and yaml-rust2
  • 0.9: no interop between A and yaml-rust2
  • >= 0.8, < 0.10: same as 0.9

All those scenarii have an iterop issue. In that regard I would say all those specifications are equal.

However, if someone uses just hashlink and yaml-rust2, using the 3rd option removes a duplicate in the binary.


I am more than willing to change the requirement back to 0.8 or to 0.9. But I would like to understand where my reasoning falls short. What's the missing piece in my above reasoning? What would you consider to be the best version requirement?

@dtolnay
Copy link

dtolnay commented Jul 14, 2024

Better example of a problematic dependency graph:

  • yaml-rust2 0.8 depends on hashlink ">= 0.8, < 0.10"
  • A depends on yaml-rust2 0.8 and hashlink 0.8, passes hashlink 0.8 LinkedHashMap to Yaml::Hash
  • B depends on yaml-rust2 0.8 and hashlink 0.9, passes hashlink 0.9 LinkedHashMap to Yaml::Hash
  • C depends on A and B

I don't think there is any way this can compile. Compiling A and B individually (with no other dependency on hashlink in the program) would resolve the way that you imagine. But during a build of C, randomly either A will not compile or B will not compile.

Instead, if:

  • yaml-rust2 0.8 depends on hashlink 0.8
  • yaml-rust2 0.9 depends on hashlink 0.9
  • A depends on yaml-rust2 0.8 and hashlink 0.8
  • B depends on yaml-rust2 0.9 and hashlink 0.9
  • C depends on A and B

there is no such issue.

@Ethiraric
Copy link
Owner

I apologize for the 3 months delay. I have run cargo update and tagged a new release.

Thank you very much @dtolnay for your input! <3

I'm going to close this issue. Do feel free to comment or re-open as needed!

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

5 participants