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

Assistance with de-coupling vendored libraries in v1.9.0 #7980

Closed
Ashvith10 opened this issue Feb 16, 2025 · 12 comments
Closed

Assistance with de-coupling vendored libraries in v1.9.0 #7980

Ashvith10 opened this issue Feb 16, 2025 · 12 comments

Comments

@Ashvith10
Copy link

I was trying to bump the broken build for Surge v1.7.0 to v.19.0 in Guix, however, it fails the build:

-- Detecting C compile features - done
-- Escape is a possibility...
CMake Error at CMakeLists.txt:131 (add_subdirectory):
  The source directory

    /tmp/guix-build-surge-synth-1.9.0.drv-0/source/libs/libsamplerate

  does not contain a CMakeLists.txt file.


-- Could NOT find Git (missing: GIT_EXECUTABLE) 
CMake Warning at cmake/versiontools.cmake:38 (message):
  Could not determine Git branch, using placeholder.


CMake Warning at cmake/versiontools.cmake:42 (message):
  Could not determine Git commit hash, using placeholder.


-- Setting up surge version
--   git hash is git-no-commit and branch is git-no-branch

Most probably because it is fetching gitmodules through CMake and not directly Git itself at the initial phase.

What I have though of next, is to remove this vendored dependency and patch the CMakeList.txt to use packaged libraries.

Guix provides libsamplerate:

$ ./pre-inst-env tree $(guix build libsamplerate)
/gnu/store/yjhs6rn1d3lpj4y04q1s1gxyw4z1ljsj-libsamplerate-0.1.9
├── bin
│   └── sndfile-resample
├── etc
│   └── ld.so.cache
├── include
│   └── samplerate.h
├── lib
│   ├── libsamplerate.a
│   ├── libsamplerate.la
│   ├── libsamplerate.so -> libsamplerate.so.0.1.8
│   ├── libsamplerate.so.0 -> libsamplerate.so.0.1.8
│   ├── libsamplerate.so.0.1.8
│   └── pkgconfig
│       └── samplerate.pc
└── share
    └── doc
        ├── libsamplerate-0.1.9
        │   └── COPYING
        └── libsamplerate0-dev
            └── html
                ├── api_callback.html
                ├── api_full.html
                ├── api.html
                ├── api_misc.html
                ├── api_simple.html
                ├── download.html
                ├── faq.html
                ├── history.html
                ├── index.html
                ├── license.html
                ├── lists.html
                ├── quality.html
                ├── SRC.css
                ├── SRC.png
                └── win32.html

11 directories, 25 files

which I can either bump to, or create a package variant of, at v0.2.2.

What should I replace add_subdirectory(libs/libsamplerate EXCLUDE_FROM_ALL) with?

@baconpaul
Copy link
Collaborator

baconpaul commented Feb 16, 2025

It sounds like when you upgraded from 1.7 to 1.9 you didn't run git submodules update recursive which is why sample rate isn't found.

i think you should replace the add_subdirectory line with nothing! but check out the code completely before you build it.

I'm personally unable to provide support to people who change the build system and repackage, and also surge 1.9.0 is the 4 year old version of surge, so its already out of anything other than most basic support.

@baconpaul
Copy link
Collaborator

baconpaul commented Feb 16, 2025

If you don't use git to checkout, you should grab the source tarball (https://github.com/surge-synthesizer/releases/releases) which contains libsamplerate with a cmakelists. Make sure to grab the 'SurgeSrc.tgz' file (https://github.com/surge-synthesizer/releases/releases/download/1.9.0/SurgeSrc_1.9.0.tgz) since that has the submodule set expanded

Good luck!

@Ashvith10
Copy link
Author

Ashvith10 commented Feb 16, 2025

It sounds like when you upgraded from 1.7 to 1.9 you didn't run git submodules update recursive which is why sample rate isn't found.

But it is enabled - field recursive? is set to #t:

(define-public surge-synth
  (package
   (name "surge-synth")
   (version "1.9.0")
   (source
     (origin
       (method git-fetch)
        (uri (git-reference
               (url "https://github.com/surge-synthesizer/surge")
               (commit (string-append "release_" version))
               (recursive? #t))) ; build system expects modules to be there
               ...
))))

i think you should replace the add_subdirectory line with nothing! but check out the code completely before you build it.

A de-coupled library can be shared amongst other programs (for example, libsamplerate is shared by many other libraries, like zrythm, qt, pulseaudio, etc) that are dependent on them, and in a way, we would not have to build libraries for every n number of packages that is dependent on the same. It will also help us in making reproducible builds, and reduced resource-usage (since it has already been built, and can be shared by other packages). We also have the flexibility to create package variants.

So far, I've applied the patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4ad951d..d0c2d53 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -128,7 +128,6 @@ add_subdirectory(libs/filesystem)
 add_subdirectory(libs/tinyxml)
 add_subdirectory(libs/escape-from-vstgui)
 add_subdirectory(libs/oddsound-mts)
-add_subdirectory(libs/libsamplerate EXCLUDE_FROM_ALL)
 
 target_link_libraries(surge-shared PUBLIC
   surge::airwindows

and it seems to be progressing, however in case of MTS-ESP, it is missing build files in it's own repository.

I'm personally unable to provide support to people who change the build system

@baconpaul - I am trying to maintain the old, broken package that hasn't been updated since the last year. Since surge and surge_xt are different, I wanted to include the final version of surge for compatibility reasons. Or I could remove surge (since it is a broken package anyway) and retry my old effort at packaging surge_xt.

@mkruselj
Copy link
Collaborator

git fetch is not gonna update submodules, AFAIK.

@baconpaul
Copy link
Collaborator

yeah the errors you are facing really sound like it didn't recursively checkout.

does the lib/samplerate and lib/MTS-ESP directory contain anything after you check out?

I understand the devendor/vendor arguments, and why you are desirous of changing our code, our packages, our versions and our make files. The wonderful thing about open source is you are free to figure that out!

But it does mean i really can't offer you any support on the resulting binary or build. Especially since until 30 minutes ago I had never even heard of guix I don't think!

@baconpaul
Copy link
Collaborator

Oh I see I had heard of guix when you asked before

I dunno. First thing to try is building unchanged with all the code in situ and no patches applied. If that doesn't work then patches aren't going to help. And it really sounds like you don't have the code base complete and available to you at build time.

@Ashvith10
Copy link
Author

Ashvith10 commented Feb 16, 2025

I dunno. First thing to try is building unchanged with all the code in situ and no patches applied.

If by in-situ, you mean running commands in the directory, then I am able to build it half-way - it fails because of the missing constant:

[ 61%] Building CXX object CMakeFiles/surge-headless.dir/src/headless/Player.cpp.o
[ 61%] Building CXX object CMakeFiles/surge-headless.dir/src/headless/UnitTests.cpp.o
In file included from /gnu/store/dl748qhs993spx2mmaj2qw1x5kzji7fy-profile/include/signal.h:328,
                 from /home/ashvith/Desktop/surge/libs/catch2/include/catch2/catch2.hpp:7714,
                 from /home/ashvith/Desktop/surge/src/headless/UnitTests.cpp:3:
/home/ashvith/Desktop/surge/libs/catch2/include/catch2/catch2.hpp:10455:58: error: call to non-'constexpr' function 'long int sysconf(int)'
10455 |     static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;
      |                                                          ^~~~~~~~~~~
In file included from /gnu/store/dl748qhs993spx2mmaj2qw1x5kzji7fy-profile/include/bits/sigstksz.h:24,
                 from /gnu/store/dl748qhs993spx2mmaj2qw1x5kzji7fy-profile/include/signal.h:328,
                 from /home/ashvith/Desktop/surge/libs/catch2/include/catch2/catch2.hpp:7714,
                 from /home/ashvith/Desktop/surge/src/headless/UnitTests.cpp:3:
/gnu/store/dl748qhs993spx2mmaj2qw1x5kzji7fy-profile/include/unistd.h:640:17: note: 'long int sysconf(int)' declared here
  640 | extern long int sysconf (int __name) __THROW;
      |                 ^~~~~~~
In file included from /home/ashvith/Desktop/surge/src/headless/UnitTests.cpp:3:
/home/ashvith/Desktop/surge/libs/catch2/include/catch2/catch2.hpp:10514:45: error: size of array 'altStackMem' is not an integral constant-expression
10514 |     char FatalConditionHandler::altStackMem[sigStackSize] = {};
      |                                             ^~~~~~~~~~~~
make[3]: *** [CMakeFiles/surge-headless.dir/build.make:272: CMakeFiles/surge-headless.dir/src/headless/UnitTests.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:492: CMakeFiles/surge-headless.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:527: CMakeFiles/all-components.dir/rule] Error 2
make: *** [Makefile:261: all-components] Error 2

And it really sounds like you don't have the code base complete and available to you at build time.

Looks very similar to #RPCS3/rpcs3/7508 - and it has been abandoned.

@baconpaul
Copy link
Collaborator

right. so in the 4 years since we last updated surge 1.9 we have updated catch to build with the latest glibcs.

Can you start from a source tarball as opposed to from a git checkout? The source tarball in our releases point has the entire source base.

You can skip the unit tests if you want. There's no real need for them for the end product. So just build the targets for surge, although I don't remember those target names.

but if guix isn't going to check out the code to be built, that does explain why it wont build it! Although i'm not sure I can do much about that.

@Ashvith10
Copy link
Author

Ashvith10 commented Feb 17, 2025

@baconpaul I had to apply this patch on top of catch2, and patch the script ./scripts/linux/package-vst3.sh:

diff --git a/scripts/linux/package-vst3.sh b/scripts/linux/package-vst3.sh
index 2f11281..617bd81 100755
--- a/scripts/linux/package-vst3.sh
+++ b/scripts/linux/package-vst3.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash

Now it builds successfully against glibc:

$ tree build -L 1
build
├── CMakeCache.txt
├── CMakeFiles
├── cmake_install.cmake
├── CPackConfig.cmake
├── CPackSourceConfig.cmake
├── geninclude
├── libs
├── libsurge-lv2-dll.so
├── libsurge-shared.a
├── libsurge-vst3-dll.so
├── lintemp
├── Makefile
├── surge-headless
└── surge_products

6 directories, 9 files

@baconpaul
Copy link
Collaborator

great! I'm glad you figured it out!

@Ashvith10
Copy link
Author

Unfortunately, this still does not work.

The built binaries do not work in host software, most probably because of broken RPATH, and even if it did, it would have to use something like a manifest file to preserve the dependencies used in the ephemeral environment to build the binaries, as well as the required patches to deal with incompatible glibc for catch2 and the NixOS/Guix #!/bin/bash incompatibility.

I will try decoupling the package on the next weekend and try adding them as input and see if they work.

@baconpaul
Copy link
Collaborator

the program should be mostly statically linked if you use the libraries we provide and don't sub them out to shared libs. other than x or libc of course.

anyway good luck!

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

No branches or pull requests

3 participants