-
Notifications
You must be signed in to change notification settings - Fork 102
Opt-in support for JACK backend on Windows #98
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
base: master
Are you sure you want to change the base?
Changes from all commits
5a2fde0
b20cac0
2088841
976ca09
b962ab2
7e79a8c
6d27093
5e5eed9
6e1a370
044792c
2575a68
7680d23
adaa5ca
8f3bf15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ license = "MIT" | |
| default = [] | ||
| avoid_timestamping = [] | ||
| jack = ["jack-sys", "libc"] | ||
| winjack = ["jack"] | ||
| winrt = [ | ||
| "windows/Foundation", | ||
| "windows/Foundation_Collections", | ||
|
|
@@ -31,7 +32,7 @@ winrt = [ | |
| [dependencies] | ||
| bitflags = "1.2" | ||
| memalloc = "0.1.0" | ||
| jack-sys = { version = "0.2", optional = true } | ||
| jack-sys = { version = "0.2.3", optional = true } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this upgrade have any incompatible changes that may affect downstream crates?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes themselves shouldn't break anything, just formatting, replacing some libc types with primitive equivalents, add a few other jack fns, and Windows support (though it does require a 2018 edition-compatible compiler). I can loosen this to
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I assume from your comment that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 0.2.3 is the first version of jack-sys that builds with pipewire-jack.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies for the delay, haven't had a chance to work on this lately. |
||
| libc = { version = "0.2.21", optional = true } | ||
|
|
||
| [target.'cfg(target_os = "linux")'.dependencies] | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,12 +1,17 @@ | ||||||||||||||||||||||||
| //! This file contains automated tests, but they require virtual ports and therefore can't work on Windows or Web MIDI ... | ||||||||||||||||||||||||
| #![cfg(not(any(windows, target_arch = "wasm32")))] | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #![cfg(not(any(all(windows, not(feature = "winjack")), target_arch = "wasm32")))] | ||||||||||||||||||||||||
| extern crate midir; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[cfg(all(windows, feature = "winjack"))] | ||||||||||||||||||||||||
| #[link(name = "C:/Program Files/JACK2/lib/libjack64")] | ||||||||||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this test be run on CI? I.e., can you add "Jack on Windows" to the CI configuration (see midir/azure-pipelines-template.yml Lines 28 to 38 in 85eaa46
That would require installing the Jack dependency on the CI machine ...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave this a go in the latest commit
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't quite tell what the L75 issue is, but L34 and L73 should be fixed by 7680d23 |
||||||||||||||||||||||||
| extern "C" {} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| use std::thread::sleep; | ||||||||||||||||||||||||
| use std::time::Duration; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| use midir::{MidiInput, MidiOutput, Ignore, MidiOutputPort}; | ||||||||||||||||||||||||
| use midir::os::unix::{VirtualInput, VirtualOutput}; | ||||||||||||||||||||||||
| use midir::ext::{VirtualInput, VirtualOutput}; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn end_to_end() { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you create a new feature and didn't reuse the
jackfeature directly? On Windows it has no meaning so far.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way to avoid surprises/breaks for any downstream crates that have the
jackfeature enabled, but expect/handle a non-jack backend on Windows. With onlyjackenabled, Windows will still use the winmm backend. I'm open to just reusingjackif this isn't a concern (I think it would clean up some of the cfg guards)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be even better if midir could pick between backends at runtime. Of course, that's a lot more work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is already tracked in #4 (but yes, a lot of work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that a semver breaking release is already being prepared, should we go ahead just use the
jackfeature for windows?