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

resolve mingw-related build issues by updating windows crate #152

Closed
j-n-f opened this issue Apr 20, 2024 · 4 comments
Closed

resolve mingw-related build issues by updating windows crate #152

j-n-f opened this issue Apr 20, 2024 · 4 comments

Comments

@j-n-f
Copy link
Contributor

j-n-f commented Apr 20, 2024

While working on #151, it was necessary to upgrade the version of the windows crate to resolve mingw-related issues. Unfortunately this causes problems in winrt builds:

error[E0277]: the trait bound `IClosable: From<windows::Devices::Midi::MidiInPort>` is not satisfied
   --> src\backend\winrt\mod.rs:164:47
    |
164 |         let closable: IClosable = self.port.0.try_into().unwrap();
    |                                               ^^^^^^^^ the trait `From<windows::Devices::Midi::MidiInPort>` is not implemented for `IClosable`, which is required by `windows::Devices::Midi::MidiInPort: TryInto<_>`
    |
    = note: required for `windows::Devices::Midi::MidiInPort` to implement `Into<IClosable>`
    = note: required for `IClosable` to implement `TryFrom<windows::Devices::Midi::MidiInPort>`
    = note: required for `windows::Devices::Midi::MidiInPort` to implement `TryInto<IClosable>`

error[E0277]: the trait bound `IClosable: From<windows::Devices::Midi::IMidiOutPort>` is not satisfied
   --> src\backend\winrt\mod.rs:266:45
    |
266 |         let closable: IClosable = self.port.try_into().unwrap();
    |                                             ^^^^^^^^ the trait `From<windows::Devices::Midi::IMidiOutPort>` is not implemented for `IClosable`, which is required by `windows::Devices::Midi::IMidiOutPort: TryInto<_>`
    |
    = note: required for `windows::Devices::Midi::IMidiOutPort` to implement `Into<IClosable>`
    = note: required for `IClosable` to implement `TryFrom<windows::Devices::Midi::IMidiOutPort>`
    = note: required for `windows::Devices::Midi::IMidiOutPort` to implement `TryInto<IClosable>`

It's not clear why this is happening, but this work seems separate from solving the re-entrant locking issue in #150 .

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

@Boddlnagg do you know why this is happening only for winrt? Just based on the types involved, I'm guessing

  1. midir needs an update to Cargo.toml so that the right features are added for the winrt feature, or
  2. this is an issue that needs to be resolved in the windows crate

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

All right, this was simpler than I thought. We no longer need to turn the midi ports into IClosable to close. The windows crate has wrapped everything up so we can just call .Close() on the midi ports themselves.

j-n-f added a commit to j-n-f/midir that referenced this issue Apr 20, 2024
* Mutexes in Windows are re-entrant, but `std::sync::Mutex` is not.
* If a sysex message is inbound when closing a MIDI input windows will
  hang because part of the call to `midiInReset` will cause the input
  handler to fire, but the `HMIDIIN` lock was already taken before that
  handler attempts to take the lock a second time.
* All of this takes place in a single thread, so there's no risk in
  allowing the lock to be taken twice.
* `parking_lot::ReentrantMutex` is used in place of `Mutex`.
* This change should behave more like `rtmidi` (which some of this code
  is based on) as `rtmidi` is also using a re-entrant mutex.
* Tested on Windows 10, and issue appears resolved.
* Update `windows` from `0.43` to `0.56` to resolve mingw-specific
  issues.
* Minor changes to `winrt` code to accomodate update to `windows`
  crate.

Fixes Boddlnagg#150
Fixes Boddlnagg#152
@Boddlnagg
Copy link
Owner

Boddlnagg commented Apr 21, 2024

In what way does this relate to the build failures (timeouts) in #146 ? (The .close() calls have been adjusted there already.)

Edit: Is that exactly the hangs that are fixed by using reentrant mutexes? Edit2: No that cannot be, because the reentrant mutexes only affect the WinMM backend ...

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 21, 2024

Hard to say. The windows crate is so complicated that when I update it and my issue is resolved, I'm not too concerned with why. I was upgrading windows to resolve an issue with the mingw build. Debugging the issue there isn't worth my time.

I think the changes now in #151 would also resolve #146. It seems the build hangs for 0.54 (#146), but not for 0.56 (#151).

#151 is ready to merge, and all tests/builds are passing (I got that fixed today).

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

2 participants