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

Link against existing libfaust #12

Open
dvzrv opened this issue Aug 4, 2020 · 15 comments · May be fixed by #13
Open

Link against existing libfaust #12

dvzrv opened this issue Aug 4, 2020 · 15 comments · May be fixed by #13

Comments

@dvzrv
Copy link

dvzrv commented Aug 4, 2020

Hi! As @agraef has brought this external to the AUR I would like to evaluate whether it is possible to link it against an already existing faust library.

I package faust for Arch Linux and there build libfaust.so and libfaust.a. This project builds a vendored version of faust (i.e. libfaust.a AFAIK) and then links against it statically.

From a packaging perspective it would be great to not build faust and/or llvm when building this external (as we already have the static and shared libraries available in a package) and instead link against libfaust.so or libfaust.a (the latter is less preferred as tracking is not possible and rebuilds are not ensured on soname bumps).

Is there anything in the general design of pd externals that would prevent something like this?

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

@dvzrv The easiest way to do this right now is to just patch up the toplevel CMakeLists.txt file so that it skips building the included Faust and links against an installed libfaust instead, like so:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2767981..147dd6e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -13,7 +13,6 @@ if(UNIX AND NOT APPLE)
     set(FAUST_LIBS  "stdc++"  CACHE STRING  "FAUST LIBRARIES" FORCE)
 endif()
 
-include(FaustLib.cmake)
 ## Create Faust~
 message(STATUS "faustgen~ external")
 
@@ -31,9 +30,7 @@ ${PROJECT_SOURCE_DIR}/src/faust_tilde_options.c)
 add_pd_external(faustgen_tilde_project faustgen~ "${faustgen_tilde_sources}")
 
 ## Link the Pure Data external with faustlib
-include_directories(${PROJECT_SOURCE_DIR}/faust/architecture)
-add_dependencies(faustgen_tilde_project staticlib)
-target_link_libraries(faustgen_tilde_project staticlib)
+target_link_libraries(faustgen_tilde_project "-lfaust")
 
 ## Link the Pure Data external with llvm
 find_package(LLVM REQUIRED CONFIG)

You then also want to copy the library (.lib) files from the installed Faust (faustgen~ expects these in the libs subdirectory of the external). Here is a modified PKGBUILD which does all this:

pd-faustgen.zip

The above will link against the shared library, so faust then needs to be a runtime dependency. It's possible to link against the static library by modifying the target_link_libraries to:

target_link_libraries(faustgen_tilde_project "/usr/lib/libfaust.a" "-lLLVM-10")

This allows the faust package to be relegated to a build dependency, which might be a slight advantage for pd-faustgen users who don't want to use the stand-alone compiler. But this also makes the package bigger, of course, and puts an additional maintenance burden on the package maintainer as the link line will have to be modified for each new LLVM version. I think that the first solution above is simpler and tidier, so it's probably the way to go.

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

From a packaging perspective it would be great to not build faust and/or llvm when building this external (as we already have the static and shared libraries available in a package) and instead link against libfaust.so or libfaust.a (the latter is less preferred as tracking is not possible and rebuilds are not ensured on soname bumps).

Well, an soname bump in LLVM would also affect a large number of other packages, and surely this would at the very least require a bump of the revision of the LLVM package, which in turn should trigger a rebuild of all packages which depend on it, no?

But if you decide to go that way instead, I've discussed the necessary changes above. Basically, you just need to switch out the patch for the PKGBUILD above with this one:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2767981..0a73518 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -13,7 +13,6 @@ if(UNIX AND NOT APPLE)
     set(FAUST_LIBS  "stdc++"  CACHE STRING  "FAUST LIBRARIES" FORCE)
 endif()
 
-include(FaustLib.cmake)
 ## Create Faust~
 message(STATUS "faustgen~ external")
 
@@ -31,9 +30,7 @@ ${PROJECT_SOURCE_DIR}/src/faust_tilde_options.c)
 add_pd_external(faustgen_tilde_project faustgen~ "${faustgen_tilde_sources}")
 
 ## Link the Pure Data external with faustlib
-include_directories(${PROJECT_SOURCE_DIR}/faust/architecture)
-add_dependencies(faustgen_tilde_project staticlib)
-target_link_libraries(faustgen_tilde_project staticlib)
+target_link_libraries(faustgen_tilde_project "/usr/lib/libfaust.a" "-lLLVM-10")
 
 ## Link the Pure Data external with llvm
 find_package(LLVM REQUIRED CONFIG)

@pierreguillot
Copy link
Member

Hi!

When I started this project, there was no pre-built version of the Faust library, it seemed better and more convenient to integrate Faust as a submodule and to compile it with the external. I guess I could have done the same for LLVM but as LLVM already offered pre-built libraries (and as the compilation longs more than 1 hours on Windows...) I prefer not to.

But as there are pre-built Faust libraries available now, we can try to link against these libraries. But, if possible, I would like to keep the compilation consistent on all the target OS (Linux/Mac/Windows). And I'm not sure that using the pre-built libraries on the CI services (Travis and Appveyor) will work. A solution could be to add an option to disable the Faust compilation and use the Faust library.

I think that using a static link is better for Pd externals because most of the users install externals using Pd package manager (Deken) and I don't want to ask users to install the Faust library and LLVM on their machine, I would like that the external remains directly usable by users. But here again, we could add an option for dynamic linking.

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

+1 for an option to link against an installed libfaust, either static or dynamic.

About the Deken thing: Personally, I think that it is pretty much broken by design, because you're stuck with whatever system libraries the submitter linked against, and it won't install any missing system libraries either. Therefore I would always prefer a packaged external over one in Deken, but that's just me.

Anyway, if you're building a package for Deken, presumably you'd build it straight from the source using the standard build procedure (and not against an installed libfaust) in which case it will be linked statically. So I don't see it as a problem if you're linking against an installed libfaust.so as an option. But I'm fine either way, and @dvzrv seems to prefer a statically linked external anyway.

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

@pierreguillot As an aside, I just noticed that there's a hidden .default.dsp file in the external dir in the sources, is that being used anywhere? I'm asking because I currently don't package this in my Arch package, so we'd still have to fix that if that file is required for faustgen~ to function properly. (But IMHO you don't want to hide essential components like this, so in that case I'd really prefer that file to be visible by renaming it to, say, default.dsp.)

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

Also, I've just submitted #13 which adds a rudimentary install target and should facilitate packaging, too. This also properly packages that hidden .default.dsp file.

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

Ok, there you go: #14. This is a quick-and-dirty solution, but it works. On Arch, it will link against the shared library, though.

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

I still have to augment #13 so that it copies the right library files if INSTALLED_FAUST is ON. I'll do that once #14 has been reviewed and merged.

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

To facilitate things and get this out of the door more quickly, you can find the entire shebang in #13 now (this also includes the commit from #14 to add the option for linking against an installed libfaust, so I closed that one).

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

So, to summarize, here's what we got in #13 now:

  • an option (cached BOOL cmake variable) INSTALLED_FAUST which is OFF by default, but can be set to ON to link against an installed libfaust

  • the requisite install target to make make install and cmake --install work

The install target also takes into account INSTALLED_FAUST, so that it copies the Faust library (.lib) files from the right location (in-tree source if INSTALLED_FAUST is OFF, installed .lib files if it is ON). In the latter case, the location is auto-detected using a search for [share/]faust/all.lib, but can be overridden with the FAUSTLIB variable. Moreover, the destination directory for the external can be set with the INSTALL_DIR variable, which is taken relative to CMAKE_INSTALL_PREFIX. The default for INSTALL_DIR is lib/pd/extra/faustgen~ on Linux and other generic Unix-like systems, and just faustgen~ on Mac and Windows.

With all that machinery in place, doing the Arch package, or almost any package I can think of, should be pretty much child's play now. (Once #13 has been merged and released, that is. ;-)

@agraef
Copy link
Contributor

agraef commented Aug 5, 2020

I'm still curious about that hidden .default.dsp file in the external dir in the sources, though. @pierreguillot can you shed some light on that?

@pierreguillot
Copy link
Member

The .default.dsp file is used as a fallback when the user doesn't specify the DSP file to use. The user should be able to see the code in a text editor by clicking on the external but he shouldn't be able to modify (or overwrite the file). That why I change the permission on the file before creating the distribution package on the Travis CI. The file is hidden because I don't want the user to change it (otherwise, the number of inputs and outputs could not be consistent) or remove it (otherwise, the "default" object will not be valid).

@agraef
Copy link
Contributor

agraef commented Aug 6, 2020

Ok, I see, fair enough.

EDIT: Although, on second thought, I don't really like the idea of "security by obscurity" at work there. A power user will always be able to circumvent this, while a noob will surely find other ways to break the external, such as deleting or munging the library files. ;-) So why not just make it visible and allow power users to change the file, while also adding a comment saying something like "Please don't edit" or "Only edit at your own risk, if you know what you are doing" to scare away the utter noobs. It's not as if your computer will explode if you do something wrong there.

@agraef
Copy link
Contributor

agraef commented Aug 6, 2020

Just updated #13 so that it also covers static linking now (and will do that as default when the INSTALLED_FAUST option is ON). I'm done with this now. @pierreguillot As soon as you merge #13 and do a 0.1.3 release, I'll update my PKGBUILDs, so that @dvzrv can then take over and submit the package to the official Arch repos.

agraef added a commit to agraef/pd-faustgen that referenced this issue Aug 7, 2020
@agraef
Copy link
Contributor

agraef commented Aug 7, 2020

@dvzrv I decided to just pull ahead now and release my latest downstream version as release 0.1.2.1. I also updated the PKGBUILD at https://aur.archlinux.org/packages/pd-faustgen/ accordingly. This uses the new install target and the option to build against an installed libfaust, as you requested, so as you can see this is now as simple as it gets. Feel free to just use that version for the community package for now, and report any further packaging-related issues at my fork.

That way Pierre can take all the time he needs to review my changes, while I can just move ahead now with the other changes that I'd like to implement.

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

Successfully merging a pull request may close this issue.

3 participants