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

Rename define to JACK_ON_WINDOWS #2525

Merged
merged 1 commit into from
Mar 20, 2022

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Mar 19, 2022

Short description of changes
Renames a define.

CHANGELOG: Rename existing define to JACK_ON_WINDOWS

Context: Fixes an issue?
Renames a define. Makes it consistent with the CONFIG option.

Does this change need documentation? What needs to be documented and how?
No.

Status of this Pull Request
Ready.

What is missing until this pull request can be merged?
Ready.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@@ -107,7 +107,7 @@ win32 {
HEADERS += linux/sound.h
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should line 104 stop the build? It's not going to work, after all...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, I'll do that in another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just open an issue to let you remember :-)

Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's certainly better than before.

In general, I was thinking whether linux/ should be renamed to jack/ (or at least parts of it? haven't checked). It's really strange to add linux/ on Windows or Mac builds.

I'm also wondering whether JACK_REPLACES_ASIO/JACK_ON_WINDOWS/JACK_ON_MAC could go away at some point (after all, there's WITH_JACK and there are defines for the actual platform as well, so I don't see the point of having one extra define for JACK on each platform).

All of that exceeds the scope of this PR though, so just adding this here as a comment.

cc @henkdegroot who last touched the JACK autobuild logic at least (although not directly related to the actual code parts).

@@ -107,7 +107,7 @@ win32 {
HEADERS += linux/sound.h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say yes.

@hoffie hoffie added this to the Release 3.9.0 milestone Mar 19, 2022
@ann0see ann0see merged commit 2a4d8e0 into jamulussoftware:master Mar 20, 2022
@ann0see
Copy link
Member

ann0see commented Mar 20, 2022

Merged since it's a trivial change.

@pljones pljones deleted the rename-jack_replaces_asio branch March 21, 2022 07:36
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

Successfully merging this pull request may close these issues.

3 participants