diwic / alsa-rs

Thin but safe ALSA wrappers for Rust
Apache License 2.0
139 stars 66 forks source link

Alsa 0.6.1 release is not semver compliant #90

Closed alice-i-cecile closed 2 years ago

alice-i-cecile commented 2 years ago

89 (and the associated release) broke semver and caused our builds to fail in CI.

Users are running into the same problem with fresh builds: see this Discord thread.

Please yank the release; and let us know if there's anything we can do to help out :)

alice-i-cecile commented 2 years ago

In our tree:

    ├── bevy_audio v0.9.0-dev (https://github.com/bevyengine/bevy.git#7989cb26)
    │   ├── ...
    │   └── rodio v0.16.0
    │       ├── cpal v0.14.0
    │       │   ├── alsa v0.6.1
    │       │   │   ├── ...
    │       │   │   └── nix v0.24.2
    │       │   ├── ...
    │       │   ├── nix v0.23.1

This seems to be a repeat of #81.

alice-i-cecile commented 2 years ago

FWIW, the underlying patch looks great, and from Bevy's perspective it would be nice to have it released as 0.7.0.

est31 commented 2 years ago

@alice-i-cecile can you copy the error message into the text of your bug report? Github does not hold onto build logs of actions for too long and it's always helpful to know the source of an error once some time has passed. Same goes for discord, it needs an account to access, not really great.

bagl commented 2 years ago

Here's the error (cpal with alsa 0.6.1, works fine with 0.6.0)

error[E0308]: mismatched types                                                                                                                                                                                                                          [6/269]
   --> src/host/alsa/mod.rs:245:21                                                                                                                                                                                                                             
    |                           
244 |         let handle = match handle_result {                                                                               
    |                            ------------- this expression has type `std::result::Result<host::alsa::alsa::PCM, (host::alsa::alsa::Error, host::alsa::alsa::nix::errno::Errno)>`
245 |             Err((_, nix::errno::Errno::EBUSY)) => return Err(BuildStreamError::DeviceNotAvailable),
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |                                                                                                                                                                                                                                                          
    = note: perhaps two different versions of crate `nix` are being used?

error[E0308]: mismatched types
   --> src/host/alsa/mod.rs:246:21
    |
244 |         let handle = match handle_result {
    |                            ------------- this expression has type `std::result::Result<host::alsa::alsa::PCM, (host::alsa::alsa::Error, host::alsa::alsa::nix::errno::Errno)>`
245 |             Err((_, nix::errno::Errno::EBUSY)) => return Err(BuildStreamError::DeviceNotAvailable),
246 |             Err((_, nix::errno::Errno::EINVAL)) => return Err(BuildStreamError::InvalidArgument),
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |
    = note: perhaps two different versions of crate `nix` are being used?

error[E0308]: mismatched types
   --> src/host/alsa/mod.rs:302:21
    |
301 |         let handle = match handle_result {
    |                            ------------- this expression has type `std::result::Result<&mut host::alsa::alsa::PCM, (host::alsa::alsa::Error, host::alsa::alsa::nix::errno::Errno)>`
302 |             Err((_, nix::errno::Errno::ENOENT)) | Err((_, nix::errno::Errno::EBUSY)) => {
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |
    = note: perhaps two different versions of crate `nix` are being used?

error[E0308]: mismatched types
   --> src/host/alsa/mod.rs:302:59
    |
301 |         let handle = match handle_result {
    |                            ------------- this expression has type `std::result::Result<&mut host::alsa::alsa::PCM, (host::alsa::alsa::Error, host::alsa::alsa::nix::errno::Errno)>`
302 |             Err((_, nix::errno::Errno::ENOENT)) | Err((_, nix::errno::Errno::EBUSY)) => {
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |
    = note: perhaps two different versions of crate `nix` are being used?

error[E0308]: mismatched types
   --> src/host/alsa/mod.rs:305:21
    |
301 |         let handle = match handle_result {
    |                            ------------- this expression has type `std::result::Result<&mut host::alsa::alsa::PCM, (host::alsa::alsa::Error, host::alsa::alsa::nix::errno::Errno)>`
...
305 |             Err((_, nix::errno::Errno::EINVAL)) => {
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |
    = note: perhaps two different versions of crate `nix` are being used?

error[E0308]: mismatched types
   --> src/host/alsa/mod.rs:720:36
    |
720 |         Err(err) if err.errno() == nix::errno::Errno::EPIPE => {
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |
    = note: perhaps two different versions of crate `nix` are being used?
error[E0308]: mismatched types
   --> src/host/alsa/mod.rs:803:40
    |
803 |             Err(err) if err.errno() == nix::errno::Errno::EPIPE => {
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `host::alsa::alsa::nix::errno::Errno`, found enum `nix::errno::Errno`
    |
    = note: perhaps two different versions of crate `nix` are being used?
mbodmer commented 2 years ago

I'd be willing to help, but it's not clear to me why the update of nix broke SemVer. Is it because CPAL did not use the alsa-rs errors in the last release yet? Wouldn't it be a problem if upstream used the re-exports?

mbodmer commented 2 years ago

In https://github.com/diwic/alsa-rs/pull/89 I've tried to introduce a verson range (greater than 0.24 using the caret. Re-reading the Cargo documentation the caret is not what I thought: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#caret-requirements, so actually this did not do what I've intended, but this shouldn't also be the cause for the semver incompatibility. As it did not change anything.

est31 commented 2 years ago

Major updates of crates that are exposed in your API are semver breaking. It's less a problem of your PR @mbodmer but of the release after that. The issue is that other crates might themselves depend on that crate that had the major update (like nix in this instance) and use the APIs in a way such that they expect the types to be compliant to each other. However, types from two versions of the same crate are not compliant to each other.

est31 commented 2 years ago

Until there is a yank you can put this into your Cargo.toml to work around the bug:

alsa = "=0.6.0"

(even if you are not using alsa directly but something like rodio or bevy).

Stuff like cargo update -p alsa --precise 0.6.0 also works but only until the next cargo update.

diwic commented 2 years ago

@alice-i-cecile @est31 I've tried to avoid this problem by re-exporting errno from nix, so you should depend on that re-export instead of depending on the alsa crate to follow a specific nix version.

I don't want to break people's code, but in this case it seems like people were doing the wrong thing, that happened to work by an accidental match of nix versions, and now you're asking me to paper over that. Right?

mbodmer commented 2 years ago

Yes, at least in CPAL it is changed now and will land in the next release: https://github.com/RustAudio/cpal/pull/711/files

est31 commented 2 years ago

@diwic oh that's a good point... I wasn't aware of this wish by the alsa maintainers to use the reexports, so cpal could improve, and there is already a PR open for it (thanks @mbodmer for that :) ). For alsa-rs, I think having reexports is a good idea, but in addition to that it is still important to do major updates of alsa once a nix version is bumped as some users might not use the reexports.

alice-i-cecile commented 2 years ago

From our end in Bevy, we only consume this indirectly, so we'll defer to cpal's solution here. @mbodmer do you have an ETA on that release?

diwic commented 2 years ago

@est31 It is also written explicitly in the documentation :

Use these re-exports instead of also depending on the nix crate. There is no guarantee that these will match a specific nix version, it may change between minor updates of the library.

I believe that most people are using the alsa crate through CPAL so if CPAL is fixed and a new version released then this should not be much of a problem in practice.

est31 commented 2 years ago

@alice-i-cecile I'm working on a fix of cpal right now. Only 0.14.0 is affected, as master is already fixed, but current master can't be released a 0.14.1. I definitely don't want to keep 0.14.0 users in this situation, so I've created a new branch with some backports of the fixes. I'll keep you updated.

alice-i-cecile commented 2 years ago

Awesome, thank you very much!

est31 commented 2 years ago

@alice-i-cecile 0.14.1 is out now!

est31 commented 2 years ago

@diwic could you please still yank version 0.6.1 and release it as 0.7.0? For future releases, cpal will not depend on nix directly, but the 0.13 branch of cpal is also affected by this. A yank would be easier I think.

diwic commented 2 years ago

@est31 Hmm, to ensure this is not happening again, can I do something to make the errno definitely incompatible with any nix version in 0.7? So that I actually can release a 0.7.1 when someone comes asking for another nix update in a year or two?

est31 commented 2 years ago

There is sadly no mechanism outside of creating wrapper types that have nothing to do with the nix crate. If the errno is the only type from the nix crate that your API exposes, you could also try to instead expose a c_int from the libc crate, as the nix crate wraps that. Then people can match on that using libc's constants (with less type safety, but there is no type issue then).

diwic commented 2 years ago

@est31 Ok, 0.7.0 is released and 0.6.1 yanked now. But any CPAL version depending on 0.7+ must use the reexport, not depending on the nix crate. Ok?

est31 commented 2 years ago

@diwic Thank you for the release. cpal will use the reexport going forward. It already does on master and on 0.14.1. I mainly asked this for the older cpal releases like the 0.13 branch which also depends on alsa 0.6, as well as the other crates that depend on alsa.

diwic commented 2 years ago

I had a look at the topmost crates depending on alsa and they don't seem to be depending on nix. The exception being termusic, which has copied the entire of CPAL into its own repo...