[bug#41053] [PATCH 1/2] gnu: Add ableton-link.

[Marius Bakke <mbakke@fastmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7605 bytes --]

Giacomo Leidi <goodoldpaul@autistici.org> writes:

> * gnu/packages/audio.scm (ableton-link): New variable.
> * gnu/packages/patches/ableton-link-system-libraries-debian.patch: Patch
> CMakeLists.txt to use system libraries.
> * gnu/local.mk (dist_patch_DATA): Add it.

[...]

> +(define-public ableton-link
> +  (package
> +    (name "ableton-link")
> +    (version "3.0.2")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/Ableton/link.git")
> +                    (commit (string-append "Link-" version))))
> +              (file-name (git-file-name name version))
> +              (sha256
> +               (base32
> +                "0262vm0v7hmqjhqx5xikh529p3c065p1yld6ymaiz74yq1dnnjir"))
> +              (modules '((guix build utils)))
> +              (patches
> +               (search-patches "ableton-link-system-libraries-debian.patch"))
> +              (snippet
> +               '(begin
> +                  ;; Fix tests
> +                  (substitute* "ci/run-tests.py"
> +                    (("root_dir,") "root_dir, os.pardir,"))

Can you expand on the comment with what this substitution does, i.e. how
it fixes tests?

> +    (native-inputs
> +     `(("catch" ,catch-framework)
> +       ("python" ,python)       ;; For running tests
> +       ("portaudio" ,portaudio) ;; For portaudio examples
> +       ("qtbase" ,qtbase)       ;; For Qt examples

Nit-pick: only one semicolon for margin comments.  We typically also do
not add a space between the semicolon and the comment in that case.

> +       ("qtdeclarative" ,qtdeclarative)
> +       ("qttools" ,qttools)))
> +    (inputs
> +     `(("jack" ,jack-1)                       ;; For JACK examples
> +       ("qtquickcontrols" ,qtquickcontrols))) ;; For Qt examples
> +    (propagated-inputs
> +     `(("asio" ,asio)))

Can you add a comment about why asio is propagated?  We try to avoid
propagation where possible.

> +    (arguments
> +     `(#:configure-flags
> +       '("-DBUILD_TYPE=Release"

You can use #:build-type instead of passing -DCMAKE_BUILD_TYPE.  Is
there a particular reason why the default "RelWithDebInfo" does not cut
it?

> +         "-DLINK_BUILD_QT_EXAMPLES=ON"
> +         "-DLINK_BUILD_JACK=ON")
> +       #:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let* ((python (string-append (assoc-ref inputs "python")
> +                                          "/bin/python3"))
> +                   (version ,(package-version ableton-link))

You can simply unquote version where needed instead of adding a let
binding.

> +                   (run-tests (string-append "../ableton-link-"
> +                                             version

I.e.:                                           ,version

> +                                             "-checkout/ci/run-tests.py")))
> +               (and (invoke python run-tests "--target" "LinkCoreTest")
> +                    (invoke python run-tests "--target" "LinkDiscoveryTest")))))

Nit-pick: The (and ...) is unnecessary because INVOKE would throw an
exception upon failure instead of #f.

> +         (add-before 'install 'patch-cmake
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let* ((version ,(package-version ableton-link))
> +                    (source (string-append "../ableton-link-"
> +                                                version

Use ,version here too.

> +                                                "-checkout/")))
> +               (substitute* (string-append source
> +                                           "cmake_include/AsioStandaloneConfig.cmake")

Note: if you run this phase immediately after 'unpack, you don't have to
add a binding for the source directory as you can refer to this file by
just (substitute* "cmake_include/AsioStandaloneConfig.cmake").  But
perhaps that will break things?

> +                 (((string-append "\\$\\{CMAKE_CURRENT_LIST_DIR\\}/\\.\\./"
> +                                  "modules/asio-standalone/asio/include"))
> +                  (string-append (assoc-ref inputs "asio")
> +                                 "/include")))
> +               (substitute* (string-append source "AbletonLinkConfig.cmake")
> +                 (("\\$\\{CMAKE_CURRENT_LIST_DIR\\}/include")
> +                  "${CMAKE_CURRENT_LIST_DIR}/../../../include")
> +                 (("\\$\\{CMAKE_CURRENT_LIST_DIR\\}/include/ableton/Link\\.hpp")
> +                  "${CMAKE_CURRENT_LIST_DIR}/../../../include/ableton/Link.hpp"))
> +               #t)))
> +         (replace 'install
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (bin (string-append out "/bin"))
> +                    (lib-cmake (string-append out "/lib/cmake/ableton-link"))
> +                    (version ,(package-version ableton-link))
> +                    (source (string-append "../ableton-link-" version "-checkout")))

Use ,version here too.

> +               (for-each (lambda (test-file)
> +                           (delete-file test-file))
> +                         '("bin/LinkDiscoveryTest" "bin/LinkCoreTest"))
> +               (copy-recursively "bin" bin)
> +               (copy-recursively (string-append source "/include/ableton")
> +                                 (string-append out "/include/ableton"))
> +               (install-file (string-append source "/AbletonLinkConfig.cmake")
> +                             lib-cmake)
> +               (install-file (string-append source
> +                                            "/cmake_include/AsioStandaloneConfig.cmake")
> +                             (string-append lib-cmake "/cmake_include"))

Terrible that we have to install "manually" here!  But oh well.

> +               #t))))))
> +    (home-page "https://github.com/Ableton/link")
> +    (synopsis "Synchronizes musical beat, tempo, and phase across multiple applications")

s/Synchronizes/Synchronize/

> +    (description
> +     "Ableton Link is a C++ library that synchronizes musical beat, tempo, and phase
> +across multiple applications running on one or more devices.  Applications on devices
> +connected to a local network discover each other automatically and form a musical
> +session in which each participant can perform independently: anyone can start or stop
> +while still staying in time.")

Pretty cool stuff!  :-)

> +    (license license:gpl2+)))
> diff --git a/gnu/packages/patches/ableton-link-system-libraries-debian.patch b/gnu/packages/patches/ableton-link-system-libraries-debian.patch
> new file mode 100644
> index 0000000000..0c12c62546
> --- /dev/null
> +++ b/gnu/packages/patches/ableton-link-system-libraries-debian.patch
> @@ -0,0 +1,27 @@
> +Description: Drop dependencies on included 3rd-party libs
> + upstream includes git-submodules for Catch and ASIO (not found in the tarball).
> + on Debian we want to use the system provided libraries.
> +Author: IOhannes m zmölnig
> +Origin: Debian
> +Forwarded: not-needed
> +Last-Update: 2016-10-26
> +---
> +This patch header follows DEP-3: http://dep.debian.net/deps/dep3/

Great that you took care to unbundle dependencies.  Can you add a link
to where you found this patch?

Otherwise LGTM, thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]