Opt-in support for JACK backend on Windows by calvinkosmatka · Pull Request #98 · Boddlnagg/midir

calvinkosmatka · 2022-01-23T00:16:58Z

Adds winjack feature to allow using JACK as the backend on Windows (non-breaking)
moves VirtualInput and VirtualOutput out of the os module and into their own virtual module (breaking)

jack-sys doesn't provide a link to libjack on Windows, so consumers will need to supply their own. I included linking to JACK2 at the standard installation path for the integration test and examples.

Boddlnagg · 2022-01-23T17:38:52Z

Thanks for the PR! I didn't know that Jack is available on Windows, too. How commonly is this used?
I added a few review comments for you to address.

calvinkosmatka · 2022-01-23T23:31:37Z

How commonly is this used?

I suspect it's fairly uncommon, but it is at least supported by Ardour for example. As far as I know it's the only way to allow applications to create their own virtual MIDI ports on Windows, which is my use case.

I added a few review comments for you to address.

Your comments aren't showing up for me, stuck pending?

Boddlnagg · 2022-01-23T17:32:53Z

 default = []
 avoid_timestamping = []
 jack = ["jack-sys", "libc"]
+winjack = ["jack"]


Why did you create a new feature and didn't reuse the jack feature directly? On Windows it has no meaning so far.

I did it this way to avoid surprises/breaks for any downstream crates that have the jack feature enabled, but expect/handle a non-jack backend on Windows. With only jack enabled, Windows will still use the winmm backend. I'm open to just reusing jack if this isn't a concern (I think it would clean up some of the cfg guards)

It would be even better if midir could pick between backends at runtime. Of course, that's a lot more work.

That is already tracked in #4 (but yes, a lot of work).

Given that a semver breaking release is already being prepared, should we go ahead just use the jack feature for windows?

Boddlnagg · 2022-01-23T17:33:15Z

 bitflags = "1.2"
 memalloc = "0.1.0"
-jack-sys = { version = "0.1.0", optional = true }
+jack-sys = { version = "0.2.3", optional = true }


Does this upgrade have any incompatible changes that may affect downstream crates?

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 ~0.2.1

So I assume from your comment that 0.2.1 does not require a 2018 edition compiler?

0.2.3 is the first version of jack-sys that builds with pipewire-jack.

Apologies for the delay, haven't had a chance to work on this lately. 0.2.1 still requires 2018 edition (as does 0.2), but it buys a few extra versions of compatibility with, for instance CPAL I see 0.2 is now in the master branch for midir 0.8-pre, so bumping this to 0.2.3 for Pipewire and Windows shouldn't be an issue?

Boddlnagg · 2022-01-23T17:36:06Z


 use midir::{MidiInput, MidiOutput, Ignore};
-use midir::os::unix::VirtualInput;
+use midir::r#virtual::VirtualInput;


This is a bit problematic in two ways:

You need to use the raw identifier syntax because virtual is a keyword ... so every user also needs to know about and use the raw identifier syntax as well. I don't like that, although I agree that os::unix is misleading when it's not Unix only (does Jack on Windows actually support virtual ports without any further setup?).

Changing the module path here is an incompatible change. For migration, I think there should be at least a re-export of the types at the old path, if possible with deprecation warnings.

Maybe as an alternative, ext would be a better name, as that is the name often used by the standard library for OS/platform-dependent extensions.

I like ext. I tried for a while to get deprecation warnings to work on the reexports, but this is apparently a longstanding issue (see rust-lang/rust#30827)

Boddlnagg · 2022-01-23T17:37:54Z

 extern crate midir;

+#[cfg(all(windows, feature = "winjack"))]
+#[link(name = "C:/Program Files/JACK2/lib/libjack64")]


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

${{ if and(not(startsWith(parameters.name, 'Windows')), not(endsWith(parameters.name, 'WASM'))) }}:

stable-jack:

rustup_toolchain: stable-${{ parameters.target }}

features: "jack"

${{ if eq(variables['Build.SourceBranch'], 'refs/heads/master') }}:

beta-jack:

rustup_toolchain: beta-${{ parameters.target }}

features: "jack"

nightly-jack:

rustup_toolchain: nightly-${{ parameters.target }}

features: "jack"

)?
That would require installing the Jack dependency on the CI machine ...

I gave this a go in the latest commit

I see the following errors in the latest build of this PR on AzureDevOps (which might only be visible to me):

I can't quite tell what the L75 issue is, but L34 and L73 should be fixed by 7680d23

Boddlnagg · 2022-01-24T07:46:41Z

Your comments aren't showing up for me, stuck pending?

Sorry, you're right, I forgot to actually finalize the review.

Be-ing · 2022-01-28T00:01:23Z

While this may be useful for local development and use, it would not be practical to distribute applications using midir with this feature until dynamic loading is implemented upstream in jack-sys.

Be-ing · 2022-01-28T16:05:31Z

it would not be practical to distribute applications using midir with this feature until dynamic loading is implemented upstream in jack-sys.

I suppose that doesn't really matter without midir dynamically picking the backend at runtime (#4). Even if libjack is dynamically loaded, it's not practical to distribute a Windows build using midir with the JACK backend if there's no fallback when libjack isn't installed.

Boddlnagg · 2022-01-28T19:21:55Z

Just a note to myself: The PR should be merged as is (modulo the current outstanding comments), but eventually I want to do a semver-breaking change and

upgrade jack-sys to 0.2.3 or later (see also fix build with pipewire-jack #99)
remove winjack in favor of just jack (I think it would then still be possible to use platform specific Cargo dependencies to enable jack on Linux and macOS, but not on Windows, if that is what one desires)
remove the os::virtual module completely

Be-ing · 2022-02-08T18:49:13Z

Version 0.9 of the JACK bindings have been released together with version 0.3 of jack-sys. This brings major breaking changes for jack-sys because it now uses dlib instead of always linking libjack. Refer to my PR updating the documentation for details. This solves the problem of platform-specific ways to link libjack thereby obviating the need for a separate feature for Windows. So I suggest to close this PR without merging.

Boddlnagg · 2022-02-12T16:38:38Z

 bitflags = "1.2"
 memalloc = "0.1.0"
-jack-sys = { version = "0.1.0", optional = true }
+jack-sys = { version = "~0.2.1", optional = true }


Why did you specify the dependency like this? I think when the major version is 0, the tilde makes no difference. Both ~0.2.1 and 0.2.1 would mean 0.2.x with x ≥ 1.
And I assume 0.2.0 would not be sufficient?

I didn't know that! For JACK on Windows, support was only added in 0.2.1 but per the discussion above, should we make this 0.2.3 for Pipewire?

Boddlnagg · 2022-02-12T16:43:45Z

@Be-ing To me this means that we should at least make sure (once we upgrade to 0.3) that Windows supports building with jack, i.e. adding it to the CI. This PR could be reworked in that direction.
And @calvinkosmatka's concern is still valid that supporting the jack feature on Windows is a breaking change because currently it can be enabled without any effect on Windows.

Also, I assume that #102 would apply to Windows as well?

Be-ing · 2022-02-12T17:34:19Z

supporting the jack feature on Windows is a breaking change

Yes, so bump the minor version to indicate that.

Also, I assume that #102 would apply to Windows as well?

Yes, the OS doesn't matter for that.

calvinkosmatka added 6 commits January 22, 2022 17:21

experimental support for jack on windows

5a2fde0

compile error when winjack and winrt enabled

b20cac0

don't update version

2088841

update comment in virtual.rs

976ca09

cleanup

b962ab2

fix type in wrappers

7e79a8c

Boddlnagg requested changes Jan 24, 2022

View reviewed changes

calvinkosmatka added 3 commits January 25, 2022 22:14

loosen jack-sys dependency

6d27093

virtual->ext + reexport

5e5eed9

azure pipeline test

6e1a370

calvinkosmatka commented Jan 26, 2022

View reviewed changes

Comment thread azure-pipelines-template.yml Outdated

pin jack version

044792c

Boddlnagg mentioned this pull request Jan 28, 2022

fix build with pipewire-jack #99

Merged

Boddlnagg reviewed Feb 12, 2022

View reviewed changes

calvinkosmatka added 4 commits February 14, 2022 22:39

Merge remote-tracking branch 'origin/master' into feature-winjack

2575a68

fix matrix name + winjack condition in azure pipeline

7680d23

clean up pipeline

adaa5ca

remove jack version from pipeline

8f3bf15

	${{ if and(not(startsWith(parameters.name, 'Windows')), not(endsWith(parameters.name, 'WASM'))) }}:
	stable-jack:
	rustup_toolchain: stable-${{ parameters.target }}
	features: "jack"
	${{ if eq(variables['Build.SourceBranch'], 'refs/heads/master') }}:
	beta-jack:
	rustup_toolchain: beta-${{ parameters.target }}
	features: "jack"
	nightly-jack:
	rustup_toolchain: nightly-${{ parameters.target }}
	features: "jack"

Conversation

calvinkosmatka commented Jan 23, 2022

Uh oh!

Boddlnagg commented Jan 23, 2022

Uh oh!

calvinkosmatka commented Jan 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinkosmatka Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Boddlnagg commented Jan 24, 2022

Uh oh!

Uh oh!

Be-ing commented Jan 28, 2022

Uh oh!

Be-ing commented Jan 28, 2022

Uh oh!

Boddlnagg commented Jan 28, 2022

Uh oh!

Be-ing commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Boddlnagg Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Boddlnagg commented Feb 12, 2022

Uh oh!

Be-ing commented Feb 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

calvinkosmatka Jan 26, 2022 •

edited

Loading

Be-ing commented Feb 8, 2022 •

edited

Loading

Boddlnagg Feb 12, 2022 •

edited

Loading