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

drumkv1: 0.9.23 -> 1.0.0 (+fix LV2 Qt UI issues) #326887

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

drumkv1: 0.9.23 -> 1.0.0 (+fix LV2 Qt UI issues) #326887

wants to merge 2 commits into from
+44 −8

Conversation

TheRedstoneDEV-DE
Copy link

@TheRedstoneDEV-DE TheRedstoneDEV-DE commented Jul 13, 2024 •
edited
Loading

Description of changes

Updated the drumkv1 drum sampler from version 0.9.23 to 1.0.0 (changelog: https://drumkv1.sourceforge.io/drumkv1-downloads.html, couldn't find any better one) and added xorg.libX11 to the dependencies.
As this is my first pull-request I hope I didn't do anything completely wrong, met all guidelines and don't cause a huge waste of time.

Things done

  • changed version to 1.0.0 and added corresponding checksum
  • added xorg.libX11to dependencies (it is used to enable LV2 X11 UI support)
  • updated nix file in general (was not able to be built before)
  • added wrapQtAppsHook
  • update to Qt6 dependency
  • added a cmake flag to disable the experimental feature "LV2 port change request"
  • added workaround to get it to package correctly
  • (Wayland support remains disabled, because it is experimental)

After I found out that the original maintainer was gone:

  • add myself to maintainer-list.nix
  • add myself as maintainer of this package
  • 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, 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot requested a review from cillianderoiste July 13, 2024 19:47
@TheRedstoneDEV-DE
Copy link
Author

added myself as maintainer of the package since the original maintainer ("goibhniu") is gone

SuperSandro2000
SuperSandro2000 requested changes Aug 5, 2024
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please drop the merge commit from this PR.

pkgs/applications/audio/drumkv1/default.nix Outdated
@@ -21,4 +56,4 @@ mkDerivation rec {
platforms = platforms.linux;
maintainers = [ maintainers.goibhniu ];
};
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add the final newline back and configure your editor to always add that.

pkgs/applications/audio/drumkv1/default.nix Outdated
cmakeFlags = [ "-DCONFIG_LV2_PORT_CHANGE_REQUEST=false" ]; # disable experimental feature "LV2 port change request"

installPhase = let
lv2Dir = "$out/lib/lv2/drumkv1.lv2";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lv2Dir = "$out/lib/lv2/drumkv1.lv2";
lv2Dir = "$out/lib/lv2/drumkv1.lv2";

pkgs/applications/audio/drumkv1/default.nix Outdated
runHook preInstall
make install
mkdir -p ${lv2Dir}
# -- some workaround, since it always wants to install to /$out/$out -- #
Copy link
Member

Choose a reason for hiding this comment

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

That means the install location in the cmake file is wrong. Please fix that. It likely needs FULL_INSTALL_DIR or so instead. This should also be upstreamed

pkgs/applications/audio/drumkv1/default.nix Outdated
Comment on lines 47 to 48
runHook postInstall
rm -r $out/nix
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runHook postInstall
rm -r $out/nix
rm -r $out/nix
runHook postInstall

pkgs/applications/audio/drumkv1/default.nix Outdated
Comment on lines 42 to 43
runHook preInstall
make install
Copy link
Member

Choose a reason for hiding this comment

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

We should move everything into postInstall and avoid calling make install by hand.

pkgs/applications/audio/drumkv1/default.nix Outdated
nativeBuildInputs = [ pkg-config ];
nativeBuildInputs = [ pkg-config cmake qt6.wrapQtAppsHook ];

cmakeFlags = [ "-DCONFIG_LV2_PORT_CHANGE_REQUEST=false" ]; # disable experimental feature "LV2 port change request"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmakeFlags = [ "-DCONFIG_LV2_PORT_CHANGE_REQUEST=false" ]; # disable experimental feature "LV2 port change request"
cmakeFlags = [
# disable experimental feature "LV2 port change request"
"-DCONFIG_LV2_PORT_CHANGE_REQUEST=false"
];

pkgs/applications/audio/drumkv1/default.nix Outdated
libsndfile
liblo
lv2
qt6.full
Copy link
Member

Choose a reason for hiding this comment

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

Please list the package individually instead of relying on the big qt6.full

pkgs/applications/audio/drumkv1/default.nix Outdated
pname = "drumkv1";
version = "0.9.23";
version = "1.0.0";

src = fetchurl {
url = "mirror://sourceforge/drumkv1/${pname}-${version}.tar.gz";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
url = "mirror://sourceforge/drumkv1/${pname}-${version}.tar.gz";
url = "mirror://sourceforge/drumkv1/drumkv1-${version}.tar.gz";

pkgs/applications/audio/drumkv1/default.nix Outdated
@@ -1,4 +1,4 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This should be a squashed in the previous commit

pkgs/applications/audio/drumkv1/default.nix Show resolved Hide resolved
@TheRedstoneDEV-DE TheRedstoneDEV-DE force-pushed the update-drumkv1 branch 2 times, most recently from f90d512 to 223dec5 Compare August 7, 2024 16:29
@TheRedstoneDEV-DE
Copy link
Author

TheRedstoneDEV-DE commented Aug 7, 2024 •
edited
Loading

I now reworked my commits (and dropped the merge), I hope I implemented everything correctly...
I have also done some more research on the problem I had and created an issue at upstream. I also implemented a more elegant, temporary fix that could be used until upstream fixed the problem (It had to do with "CMAKE_INSTALL_LIBDIR" being set to a absolute path, which was not accounted for in upstream's CMakeLists ; I hope this isn't a problem).
update: I think ofborg-eval fails because of the conflicts in the default.nix file (this was the actual reason why I updated the branch), they shouldn't be hard to resolve (it's just one line)...
Anyways if I still somehow got something wrong, I hope I don't steel you too much time, like I wrote, this is my first contribution to nixpkgs...

@SuperSandro2000
Copy link
Member

Please rebase on master

@TheRedstoneDEV-DE TheRedstoneDEV-DE force-pushed the update-drumkv1 branch 4 times, most recently from 1590637 to ddacfbe Compare August 18, 2024 17:30
@TheRedstoneDEV-DE
Copy link
Author

Ah, thanks for reminding me, I didn't have collisions in a PR before...
I hope everything is fine now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cillianderoiste cillianderoiste Awaiting requested review from cillianderoiste

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
8.has: maintainer-list (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TheRedstoneDEV-DE @SuperSandro2000 @NixOSInfra