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

jackass: init at v0.2.3 #241989

Merged
merged 1 commit into from Mar 12, 2024
Merged

jackass: init at v0.2.3 #241989

merged 1 commit into from Mar 12, 2024

Conversation

PowerUser64
Copy link
Contributor

Description of changes

This PR adds falkTX/jackass.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
@PowerUser64
Copy link
Contributor Author

Note, I changed the meta to have multiple licenses because vst2 sdk is unfree.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Made a few more comments. Basically the same remarks but for the secondary derivation.

Please, let's be consistent when writing these even if using the finalAttrs pattern does not make a lot of difference there.

The commit log should be: jackass: init at 0.2.3

Thanks.

pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/jackass/default.nix Outdated Show resolved Hide resolved
@PowerUser64
Copy link
Contributor Author

PowerUser64 commented Jul 25, 2023

Maybe we should just package the vst2sdk on its own, it's used in quite a few places.

@drupol
Copy link
Contributor

drupol commented Jul 28, 2023

Thanks.

Are you going to create a new PR or update this one ?

@drupol drupol marked this pull request as draft July 29, 2023 06:34
@PowerUser64
Copy link
Contributor Author

Sorry, I've been away for a bit. I think I'll make it a separate PR. In it, I'll change this and all the other packages to use it.

@drupol
Copy link
Contributor

drupol commented Jul 30, 2023

Fair enough thanks!

@PowerUser64
Copy link
Contributor Author

PowerUser64 commented Aug 23, 2023

Could we merge this in its current state and then go through and make the change with the vst2 sdk afterwards? It works in its current state and it would be easiest to merge this and then add the vst2 sdk as a separate package and update everything to use it in another PR.

@PowerUser64 PowerUser64 force-pushed the jackass branch 2 times, most recently from b27b85d to 3842226 Compare August 23, 2023 05:57
@drupol
Copy link
Contributor

drupol commented Aug 23, 2023

Why not make things correctly right now so we avoid postponing and avoid introducing things that need to be done at a later moment ?

@PowerUser64
Copy link
Contributor Author

Everything with the vst2 sdk is made incorrectly right now, and I think conforming with the precedent would be fine for one last package. After that, we make a PR to make them all correct. That way we have one PR for each of the changes and we're not adding two packages in this one PR.

@drupol
Copy link
Contributor

drupol commented Aug 23, 2023

Fair enough I understand but this is not my philosophy of working. I'm for submitting the most correct thing from the beginning and avoid postponing things at a later moment.

This is my point of view, maybe someone else will have another point of view.

@PowerUser64
Copy link
Contributor Author

Whew, it's been a while, but I'm able to work on this now.

I've realized we probably shouldn't package the vst2 sdk. The reason is that the maker of vst2, Steinberg, has discontinued it, and part of the vst2 license agreement states that you can't use it to make new things or distribute it after they discontinue it. Adding it as a package to nixpkgs feels like it's in even further violation of this. It's not used in many places, and usage of the vst2 sdk has died down significantly, but the terms are still there and it's clear Steinberg doesn't want people to keep using it.

Steinberg has graciously turned a blind eye to the fact that archive.org has been hosting a copy of it that a number of linux distros (including us) have been relying on to build stuff. That or they actually don't know about it. Either way, it is still technically not legal for archive.org to distribute this. Adding a package for the vst2 sdk would make it easy to include in development environments and perpetuate this, which could draw attention to the existence of the copy on archive.org and lead to its removal.

Anyway, long story short, I think adding the vst2 sdk as a package would be ill-guided and could risk losing us access to the vst2 sdk in the first place, so I really think we shouldn't package it at all.

@tobiasBora
Copy link
Contributor

The licensing of vst2 is indeed tricky, we already discussed it here #145607 and ended up creating two versions of bespokesynth, one compiled without the vst2 and one with that is not prebuilt by hydra.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/1016/100

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Please move the package in pkgs/by-name directory.

Also, don't post your PR on discourse if it is in draft .

@PowerUser64
Copy link
Contributor Author

Oh, I didn't know I was able to change it from draft. I'll do that.

Please move the package in pkgs/by-name directory.

👍 will do

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-deal-with-two-versions-of-a-package-in-by-name/40841/1

pkgs/by-name/ja/jackass/package.nix Outdated Show resolved Hide resolved
@tobiasBora
Copy link
Contributor

@PowerUser64 is it working also without the proprietary vst? If so, you can maybe create two packages like we do for bespokesynth, one fully open source and one that is not. The most elegant solution in that case to keep by-name seems to do what this post suggests: https://discourse.nixos.org/t/how-to-deal-with-two-versions-of-a-package-in-by-name/40841/6?u=tobiasbora

@PowerUser64
Copy link
Contributor Author

@PowerUser64 is it working also without the proprietary vst? If so, you can maybe create two packages like we do for bespokesynth, one fully open source and one that is not.

That is an interesting question, it reminds me of https://github.com/Xaymar/vst2sdk, maybe I should ask falkTX (maintainer of jackass) if it could be written to include it instead of the proprietary vst sdk. However I'm guessing that's not what you were asking about. Unfortunately there aren't any versions of this plugin that don't use the proprietary vst sdk such as some other plugins do (like an lv2, vst3, or clap version). If there were other versions I'd say we should do that. I'll mention this to falk.

@tobiasBora
Copy link
Contributor

This is very interesting, I wasn't aware of its existence. Is it stable? I was just refering to the fact that if jackass also comes with vst3 or lv, we can optionally enable vst2 if the user is ready to use non-free softwares.

@PowerUser64
Copy link
Contributor Author

As far as I know, it's stable. It is being used in https://github.com/DISTRHO/DPF and there have only been 4 commits to the file since it was added (the file is here). It would be nice if maybe we could replace the vst2sdk here (or in bespoke) with this, although I'm not sure if we should because that seems like deviating pretty far from the original software.
cc @falkTX

@falkTX
Copy link

falkTX commented Mar 7, 2024

the way forward is porting the plugin to DPF. the only missing thing on DPF side is a way to fetch the host name size, which the jackass plugin uses for naming the jack client.

@drupol drupol merged commit a07fe6b into NixOS:master Mar 12, 2024
23 checks passed
@PowerUser64
Copy link
Contributor Author

Thanks for the review @drupol!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants