tafia / quick-xml

Rust high performance xml reader and writer
MIT License
1.19k stars 234 forks source link

Release 0.24.1 #475

Closed tustvold closed 2 years ago

tustvold commented 2 years ago

It would appear that 0.24.0 has just been yanked, and a new 0.25.0 release cut. Unfortunately this means released crates with a version pin of ^0.24.0 now fail to compile, as 0.25.0 is considered semver incompatible.

Reviewing the changelog for 0.25.0 I don't see anything obvious that would constitute a breaking change, and so I wonder if we might release a 0.24.1 with the change in #471 to avoid ecosystem churn? Nothing in that change appears to be breaking

FYI @Mingun

Mingun commented 2 years ago

Hm... Documentation for cargo yank stated, that crates that already uses yanked version still will able to use it. If that is not true, maybe this is a bug in cargo? It is surprise for me, that yanking breaks something. I tried to prevent using 0.23.0 and 0.24.0 in new projects.

tustvold commented 2 years ago

Perhaps that is only if pinned to that specific version, as opposed to a semver constraint, I don't honestly know? I might file a PR to the docs repo to clarify that, and in so doing find out from the people who will know :thinking:

In my experience I have frequently found yanked crates forcing me to drop everything and patch a version. It's certainly not transparent to downstreams in the way that would imply... Personally I wish crates.io didn't have the functionality, even in the case of CVEs, RUSTSEC is a better way to communicate that without forcing updates on downstreams.

For reference here is the failing CI job - https://github.com/apache/arrow-rs/runs/8286264184?check_suite_focus=true

Mingun commented 2 years ago

In your case you should update quick-xml in any case because of using quick_xml::de::from_reader, for example, here: https://github.dev/tustvold/arrow-rs/blob/2721d253ecea84b06bc890a09a67dcb389bb7ad6/object_store/src/aws/client.rs#L388

It uses buffered reading and could break is S3 response will contain CDATA elements.

adamreichold commented 2 years ago

I think the relevant part of the Cargo documentation is

Note that existing crates locked to a yanked version will still be able to download the yanked version to use it. Cargo will, however, not allow any new crates to be locked to any yanked version.

meaning that only locked versions continue to work, meaning entries in Cargo.lock. However, resolving a yanked version from a constraint in Cargo.toml does not which is why one should generally only yank versions for which semver-compatible alternatives containing the fix exist.

tustvold commented 2 years ago

You should update

Yes, agreed. All I'm asking is that we provide a way that people can update through semver, without needing new releases to be cut. Are you suggesting all downstreams should yank any releases using 0.24.0? Would it not be better to just cut a patch release that fixes the bug and let everyone get the fix for free?

Mingun commented 2 years ago

Because there are no significant changes yet, I thought that updating from 0.24 to 0.25 should not be a problem. It is possible to release a patch release also. Will release soon

meaning that only locked versions continue to work, meaning entries in Cargo.lock.

Maybe also it is just because last released version of object_store is 0.4.0 and Cargo.toml already specified 0.5.0 which is new version that should use new release. Maybe if there was 0.4.x, the error would not have arisen.

tustvold commented 2 years ago

Cargo.toml already specified 0.5.0

This is because the 0.5.0 release is in its acceptance testing phase and has already been cut, this is part of why I would greatly appreciate a patch release

Maybe if there was 0.4.x, the error would not have arisen.

You can confirm this is not the case with a simple test

$ cargo new --lib foo
$ cd foo
$ echo 'quick-xml = "0.24.0"' >> Cargo.toml
$ cargo build
error: failed to select a version for the requirement `quick-xml = "^0.24.0"`
candidate versions found which didn't match: 0.25.0, 0.22.0, 0.21.0, ...
location searched: crates.io index
required by package `foo v0.1.0 (/home/raphael/foo)`

I thought that updating from 0.24 to 0.25 should not be a problem

It wouldn't be, the issue is yanking 0.24 and not providing a semver compatible replacement :sweat_smile:

I've proposed https://github.com/rust-lang/cargo/pull/11071 to address the documentation, as I agree it is definitely misleading

Will release soon

Thank you :heart:

Mingun commented 2 years ago

You can confirm this is not the case with a simple test

I don't think so. That crate is created locally and of course it is new. If it would be already released...


Released 0.24.1

tustvold commented 2 years ago

Thank you, especially for handling this so quickly :tada:

That crate is created locally and of course it is new. If it would be already released...

We can test this using cargo-tarpaulin which depends on 0.23 which has also been yanked

$ cargo new --lib foo
$ cd foo
$ echo 'cargo-tarpaulin = "0.21.0"' >> Cargo.toml
$ cargo build
error: failed to select a version for the requirement `quick-xml = "^0.23"`
candidate versions found which didn't match: 0.25.0, 0.24.1, 0.22.0, ...
location searched: crates.io index
required by package `cargo-tarpaulin v0.21.0`
    ... which satisfies dependency `cargo-tarpaulin = "^0.21.0"` of package `foo v0.1.0 (/home/raphael/foo)`

Similarly attempting to install inferno, also fails

cargo install inferno
    Updating crates.io index
  Installing inferno v0.11.7
error: failed to compile `inferno v0.11.7`, intermediate artifacts can be found at `/tmp/cargo-installUvTt2O`

Caused by:
  failed to select a version for the requirement `quick-xml = "^0.23"`
  candidate versions found which didn't match: 0.25.0, 0.24.1, 0.22.0, ...
  location searched: crates.io index
  required by package `inferno v0.11.7`

You might want to consider releasing a patch version of 0.23 as well...