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.

Sign up for GitHub

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

Jump to bottom

overwitch, nixos/overwitch: init at 1.0 #269278

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

overwitch, nixos/overwitch: init at 1.0 #269278

wants to merge 4 commits into from
+112 −0

Conversation

dag-h
Copy link

@dag-h dag-h commented Nov 22, 2023 •
edited
Loading

Description of changes

Closes #165104.

Overwitch is an Overbridge 2 device client for JACK (JACK Audio Connection Kit).

The package requires udev/hwdb rules to be set to function properly, so these have been added in the new nixos/overwitch module. The package can conditionally be built without the GTK GUI. The package has therefore been added as both overwitch (CLI only) and overwitch-gtk (CLI and GUI).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 22, 2023
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Nov 23, 2023
@dag-h
Copy link
Author

dag-h commented Nov 25, 2023

I'm not sure how to interpret the OfBorg error. Can someone fill me in if there's something I need to fix?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@jchv
Copy link
Contributor

jchv commented Apr 29, 2024

I think the reason is because what you want is actually libusb1. But why does libusb work locally? I dunno. It still seems to work locally with just libusb even though I don't see a libusb attr anywhere and all of the other packages I saw seemed to refer to libusb1.

Are you still interested in working on/maintaining this? I don't have a device to test this with but I can make a review.

jchv
jchv approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@jchv jchv left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Package seems to build + run fine, too, to the extent that I can test it.

Note that since this PR was created, there is a new formatting style and autoformatter that can be used for nixpkgs. See the nixfmt-rfc-style package. To my knowledge, it is not necessary to adhere to this style right now, but it wouldn't hurt either.

nixos/modules/programs/overwitch.nix
};
in {
options.programs.overwitch = {
enable = mkEnableOption (lib.mdDoc "Install overwitch and add the necessary udev and hwdb rules.");
Copy link
Contributor

Choose a reason for hiding this comment

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

lib.mdDoc should no longer be necessary, see e.g. #300735

Also, you are using mkEnableOption so you may want to be mindful of the way the string will be formatted. This will read as "Whether to enable Install overwitch and add the necessary udev and hwdb rules.."

nixos/modules/programs/overwitch.nix
};

config = mkIf cfg.enable {
environment.systemPackages = [ overwitch ];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra whitespace

Suggested change
environment.systemPackages = [ overwitch ];
environment.systemPackages = [ overwitch ];

pkgs/applications/audio/overwitch/default.nix
, stdenv
, fetchFromGitHub
, libtool
, libusb
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
, libusb
, libusb1

I think this would fix the Hydra problem.

pkgs/applications/audio/overwitch/default.nix

stdenv.mkDerivation rec {
pname = "overwitch";
version = "1.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version = "1.0";
version = "1.1";

It's now version 1.1, may as well update it I think?

pkgs/applications/audio/overwitch/default.nix

src = fetchFromGitHub {
owner = "dagargo";
repo = pname;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repo = pname;
repo = "overwitch";

The pattern of using rec to pull repo from pname has become somewhat unfavored since this PR was made. Sorry to be a bother, but I'd recommend simply repeating the string "overwitch" here. For context see the finalAttrs pattern and related discussions about rec.

pkgs/applications/audio/overwitch/default.nix
src = fetchFromGitHub {
owner = "dagargo";
repo = pname;
rev = version;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rev = version;
rev = finalAttrs.version;

Likewise, probably best to just repeat the tag, or introduce the finalAttrs pattern and use finalAttrs.version here.

pkgs/applications/audio/overwitch/default.nix
, withGui ? true
}:

stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation (finalAttrs: {

Related to other comments: I recommend removing rec and possibly adopting the finalAttrs pattern in accordance with recent changes in NixOS.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 9, 2024
@JoNilsson JoNilsson mentioned this pull request May 11, 2024
Open
Are10 pushed a commit to Are10/nixpkgs that referenced this pull request Jun 26, 2024
@rbdm-dev
Update PR to resolve feedback
865bb3b 
Incorporates feedback from NixOS#269278
Sets Overwitch version to 1.1
@Are10 Are10 mentioned this pull request Jun 26, 2024
Open
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchv jchv jchv approved these changes

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Packaging request: Overwitch
4 participants
@dag-h @jchv @wegank @h7x4