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

musescore: 4.0.2 -> 4.1.0 #243298

Merged
merged 6 commits into from
Jul 18, 2023
Merged

musescore: 4.0.2 -> 4.1.0 #243298

merged 6 commits into from
Jul 18, 2023

Conversation

doronbehar
Copy link
Contributor

Diff: musescore/MuseScore@v4.0.2...v4.1.0

Description of changes

Close #243109 .

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.

@colin-heffernan
Copy link
Contributor

It's working for me.

@ofborg ofborg bot requested a review from turion July 13, 2023 17:21
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 13, 2023
@doronbehar
Copy link
Contributor Author

@ofborg build musescore

@doronbehar doronbehar force-pushed the pkg/musescore branch 2 times, most recently from 1f4dd1b to edadd2d Compare July 15, 2023 13:32
@doronbehar
Copy link
Contributor Author

cc @yurrriq per #23204 .

@doronbehar
Copy link
Contributor Author

doronbehar commented Jul 15, 2023

Aish! Syntax errors in /nix/store...-apple-framework-Foundation-11.0.0/Library/Frameworks/Foundation.framework/Headers ?? How can this be?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/need-help-from-darwin-users-syntax-errors-in-library-frameworks-foundation-framework-headers/30467/1

@doronbehar
Copy link
Contributor Author

I'll wait a few days for help from Darwin users, and if none will come, I'll do merge the previous dmg based expression by @yurrriq into default.nix.

@reckenrode
Copy link
Contributor

I’m replying here as well to document (from https://discourse.nixos.org/t/need-help-from-darwin-users-syntax-errors-in-library-frameworks-foundation-framework-headers/30467/2). The issue is portaudio is propagating frameworks from the 10.12 SDK. Mixing frameworks from different SDK versions doesn’t work. It results in errors because the headers aren’t compatible. You need to override portaudio to use the 11.0 SDK.

let
  # portaudio propagates Darwin frameworks. Rebuild it using the 11.0 stdenv from Qt and the 11.0 SDK frameworks.
  portaudio' = if !stdenv.isDarwin
    then portaudio
    else portaudio.override {
      inherit stdenv;
      inherit (darwin.apple_sdk_11_0.frameworks) AudioUnit AudioToolbox CoreAudio CoreServices Carbon;
    };
in

I also want to note this derivation doesn’t build with sandbox enabled. This is a Qt issue. qtbase should be propagating its sandbox profile, but it doesn’t actually seem to work. This is the current workaround I’ve used in a couple of other packages:

# HACK `propagatedSandboxProfile` does not appear to actually propagate the sandbox profile from `qtbase`
sandboxProfile = toString qtbase.__propagatedSandboxProfile or null;

@doronbehar
Copy link
Contributor Author

@ofborg build musescore

Thanks for the detailed answer @reckenrode ! Hopefully this will work now. I wonder why the default darwin frameworks are not 11_0 throughout all Nixpkgs... Perhaps, portaudio and portmidi should not propagate these deps?

@doronbehar
Copy link
Contributor Author

Still very similar syntax errors...

@reckenrode
Copy link
Contributor

I wonder why the default darwin frameworks are not 11_0 throughout all Nixpkgs

The x86_64-darwin SDK hasn’t been bumped in a while. @toonn is working on a bump to 10.13. As for why, I think there is a desire to use a source-based SDK when possible as well as one to support as many users as possible.

I don’t see needing to support multiple SDKs changing. Some software requires a newer one or should be built with a newer one (e.g., Blender) then back deployed (e.g., MoltenVK). Some could be built with a newer one and work okay, but presumably a lot of software does static feature detection instead of dynamic, so we have to build with as old as possible to maximize support. There are definitely some ergonomic and composability issues with the current approach (SDK-specific callPackages). I opened #242666 to discuss the future of SDKs on Darwin.

@doronbehar
Copy link
Contributor Author

I opened #242666 to discuss the future of SDKs on Darwin.

Thanks for the link! Do you have any idea what can be done with the current compilation issue? It's availalbe here.

@reckenrode
Copy link
Contributor

Thanks for the link! Do you have any idea what can be done with the current compilation issue? It's availalbe here.

portmidi doesn’t propagate its build inputs, so you don’t have to rebuild it. It also appears I was wrong about the stdenv. I thought Qt would provide an 11.0 SDK one, but it’s apparently still the default 10.12 SDK stdenv. Using the 11.0 SDK stdenv directly allows it to build for me. I apologize for not doing more testing (aside from confirming the expression for portaudio). Here’s a diff with the changes I made to get it to build for x86_64-darwin.

diff --git a/pkgs/applications/audio/musescore/default.nix b/pkgs/applications/audio/musescore/default.nix
index 07a9e36a0ab..da0c8110a66 100644
--- a/pkgs/applications/audio/musescore/default.nix
+++ b/pkgs/applications/audio/musescore/default.nix
@@ -31,10 +31,11 @@
 }:
 
 let
+  stdenv' = if stdenv.isDarwin then darwin.apple_sdk_11_0.stdenv else stdenv;
   # portaudio and portmidi propagate Darwin frameworks. Rebuild them using the
   # 11.0 stdenv from Qt and the 11.0 SDK frameworks.
   portaudio' = if stdenv.isDarwin then portaudio.override {
-      inherit stdenv;
+      stdenv = stdenv';
       inherit (darwin.apple_sdk_11_0.frameworks)
         AudioUnit
         AudioToolbox
@@ -43,17 +44,7 @@ let
         Carbon
       ;
     } else portaudio;
-  portmidi' = if stdenv.isDarwin then portmidi.override {
-      inherit stdenv;
-      inherit (darwin.apple_sdk_11_0.frameworks)
-        Carbon
-        CoreAudio
-        CoreFoundation
-        CoreMIDI
-        CoreServices
-      ;
-    } else portmidi;
-in stdenv.mkDerivation rec {
+in stdenv'.mkDerivation rec {
   pname = "musescore";
   version = "4.1.0";
 
@@ -101,7 +92,7 @@ in stdenv.mkDerivation rec {
     libsndfile
     libvorbis
     portaudio'
-    portmidi'
+    portmidi
     flac
     qtbase
     qtdeclarative

Unfortunately, it fails during install phase. At least it’s not SDK related ….

CMake Error at src/app/cmake_install.cmake:87 (file):
  file INSTALL cannot find
  "/private/tmp/nix-build-qtbase-5.15.9.drv-0/qtbase-a196623/$(out)/$(qtQmlPrefix)":
  No such file or directory.
Call Stack (most recent call first):
  src/cmake_install.cmake:42 (include)
  cmake_install.cmake:57 (include)


FAILED: CMakeFiles/install.util
cd /tmp/nix-build-musescore-4.1.0.drv-0/source/build && /nix/store/q6cc8rhv9v8ji23xw3d8rp2ns0cclzc1-cmake-3.26.4/bin/cmake -P cmake_install.cmake
ninja: build stopped: subcommand failed.

@@ -54,6 +54,9 @@ in stdenv'.mkDerivation rec {
rev = "v${version}";
sha256 = "sha256-CqW1f0VsF2lW79L3FY2ev+6FoHLbYOJ9LWHeBlWegeU=";
};
patches = [
./darwin-fix-install.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a comment on why this patch is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please add a comment on why this patch is required?

I will, once I see that it works.

@reckenrode
Copy link
Contributor

reckenrode commented Jul 15, 2023

I was able to get it to build, but I ran into a few issues.

  • It hardcodes x86_64 by default. I had to apply a fix in postPatch.
  • The app bundle should be installed to $out/Applications.
  • The wrapper args need tweaked to distinguish between Darwin and Linux.
  • It was failing to open due to missing QtQuick.Dialogs. I had to add qtquickcontrols as a buildInput.
  • It appears there is some third-party dep stuff installed to $out/lib and $out/include. Is that needed?

The following diff resolves these issues (except the last one). Testing was confirming it launched and that I could place notes on things in a document.

diff --git a/pkgs/applications/audio/musescore/default.nix b/pkgs/applications/audio/musescore/default.nix
index 56185fccf0d..4b344516c11 100644
--- a/pkgs/applications/audio/musescore/default.nix
+++ b/pkgs/applications/audio/musescore/default.nix
@@ -20,6 +20,7 @@
 , qtdeclarative
 , qtgraphicaleffects
 , flac
+, qtquickcontrols
 , qtquickcontrols2
 , qtscript
 , qtsvg
@@ -70,7 +71,8 @@ in stdenv'.mkDerivation rec {
 
   qtWrapperArgs = [
     # MuseScore JACK backend loads libjack at runtime.
-    "--prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ libjack2 ]}"
+    "--prefix ${lib.optionalString stdenv.isDarwin "DY"}LD_LIBRARY_PATH : ${lib.makeLibraryPath [ libjack2 ]}"
+  ] ++ lib.optionals (!stdenv.isDarwin) [
     # There are some issues with using the wayland backend, see:
     # https://musescore.org/en/node/321936
     "--set-default QT_QPA_PLATFORM xcb"
@@ -100,6 +102,7 @@ in stdenv'.mkDerivation rec {
     qtbase
     qtdeclarative
     qtgraphicaleffects
+    qtquickcontrols
     qtquickcontrols2
     qtscript
     qtsvg
@@ -110,6 +113,16 @@ in stdenv'.mkDerivation rec {
     alsa-lib
   ];
 
+  postPatch = lib.optionalString stdenv.isDarwin ''
+    substituteInPlace build/cmake/SetupBuildEnvironment.cmake \
+      --replace 'set(CMAKE_OSX_ARCHITECTURES x86_64)' 'set(CMAKE_OSX_ARCHITECTURES ${stdenv.hostPlatform.darwinArch})'
+  '';
+
+  postInstall = lib.optionalString stdenv.isDarwin ''
+    mkdir -p "$out/Applications"
+    mv "$out/mscore.app" "$out/Applications/mscore.app"
+  '';
+
   passthru.tests = nixosTests.musescore;
 
   meta = with lib; {

@reckenrode
Copy link
Contributor

reckenrode commented Jul 15, 2023

I did a search for a file I could open. Playback works too (though it crashes if you quit during playback).

@doronbehar
Copy link
Contributor Author

Thanks again for the help @reckenrode . Regarding the CMAKE_OSX_ARCHITECTURES x86_64 thing: So you tested this build on an m1 aarch64 darwin machine? I wonder whether we can / should contribute this fix, and my patch upstream.

Also, regarding the postInstall, Do you get eventually also a $out/bin/mscore binary? Is $out/Applications/mscore.App needed in addition to $out/bin/mscore?

@doronbehar
Copy link
Contributor Author

Oh and BTW I was wondering what kind of error do you get when sandboxProfile is not set to that qtbase.__propagatedSandboxProfile...

# Upstream assumes the user has x86_64, and not aarch64
postPatch = lib.optionalString stdenv.isDarwin ''
substituteInPlace build/cmake/SetupBuildEnvironment.cmake \
--replace 'set(CMAKE_OSX_ARCHITECTURES x86_64)' 'set(CMAKE_OSX_ARCHITECTURES ${stdenv.hostPlatform.darwinArch})'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether this can be set with a cmakeFlag (question for @reckenrode ).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is BUILD_MACOS_APPLE_SILICON, which can be set to ON. It works by clearing out CMAKE_OSX_ARCHITECTURES, which (as I understand it) means to build for the native host only. That should also work normally, but I’m not sure what it would do in a cross build.

I tried cross-building musescore (because x86_64-darwin to aarch64-darwin should work), but it appears waf needs an emulator for an aarch64-darwin cross-build, and there isn’t one available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is BUILD_MACOS_APPLE_SILICON, which can be set to ON. It works by clearing out CMAKE_OSX_ARCHITECTURES, which (as I understand it) means to build for the native host only. That should also work normally, but I’m not sure what it would do in a cross build.

Cross compilation is a very high ideal for this package IMO. I removed that substituteInPlace in favor of the mentioned cmake flag which I think will survive updates better, especially if upstream will rewrite/rename their cmake files.

@reckenrode
Copy link
Contributor

Thanks again for the help @reckenrode . Regarding the CMAKE_OSX_ARCHITECTURES x86_64 thing: So you tested this build on an m1 aarch64 darwin machine?

I tested on an M1 Max MBP.

I wonder whether we can / should contribute this fix, and my patch upstream.

I assume such a patch would need to make copying the QML files conditional (e.g., on MUE_USE_SYSTEM_QML) because upstream probably wants to continue to embed them along with the Qt frameworks.

Also, regarding the postInstall, Do you get eventually also a $out/bin/mscore binary? Is $out/Applications/mscore.App needed in addition to $out/bin/mscore?

There’s no $out/bin/mscore. It only creates the app bundle. If you also want $out/bin/mscore, you’ll need to symlink $out/applications/mscore.app/Contents/MacOS/mscore to $out/bin/mscore.

I also wanted to note there are the following things in $out. These don’t look related or required by musescore. If not, can they removed from $out?

result/include:
gmock/  gtest/  kddockwidgets/  opus/

result/lib:
cmake/  libgmock.a  libgmock_main.a  libgtest.a  libgtest_main.a  libkddockwidgets.a  libopus.a  pkgconfig/

@reckenrode
Copy link
Contributor

Oh and BTW I was wondering what kind of error do you get when sandboxProfile is not set to that qtbase.__propagatedSandboxProfile...

One of the Qt applications using during the build process will crash during when the sandbox is enabled. See #234122 for details.

In theory, the profile should be propagated to anything using qtbase. It doesn’t seem to work though (see #237458).

@doronbehar
Copy link
Contributor Author

I also wanted to note there are the following things in $out. These don’t look related or required by musescore. If not, can they removed from $out?

Defninitly, these are not darwin specific. I removed them.

There’s no $out/bin/mscore. It only creates the app bundle. If you also want $out/bin/mscore, you’ll need to symlink $out/applications/mscore.app/Contents/MacOS/mscore to $out/bin/mscore.

OK good, I created that symlink.

I assume such a patch would need to make copying the QML files conditional (e.g., on MUE_USE_SYSTEM_QML) because upstream probably wants to continue to embed them along with the Qt frameworks.

Yes I agree. I opened an issue there: musescore/MuseScore#18665 .

Other then that, I squashed all the darwin related changes to 1 commit, and I'm pretty sure it should work. Thanks once more @reckenrode for all the help.

@NixOS NixOS deleted a comment from doronbehar Jul 17, 2023
@NixOS NixOS deleted a comment from doronbehar Jul 17, 2023
@NixOS NixOS deleted a comment from doronbehar Jul 17, 2023
Co-authored-by: Randy Eckenrode <randy@largeandhighquality.com>
@doronbehar
Copy link
Contributor Author

CI is fully green right now. I think this is ready to go. Any other objections / comments?

@drupol
Copy link
Contributor

drupol commented Jul 18, 2023

There's one missing test and it's ready to merge after that. Nice team work in this PR !

@drupol
Copy link
Contributor

drupol commented Jul 18, 2023

Ok enough waiting.

@drupol drupol merged commit 446b09f into NixOS:master Jul 18, 2023
@yurrriq
Copy link
Member

yurrriq commented Jul 19, 2023

cc @yurrriq per #23204 .

@doronbehar et al. thanks for this PR! Six years ago 😮 I just did a naive port of the Homebrew... recipe? to get a quick and dirty (and binary) expression for darwin. I'm glad to see this finally getting fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: MuseScore 4.0.2 → 4.1
9 participants