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

Add an install target, and allow linking against an installed libfaust. #13

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

Conversation

agraef
Copy link
Contributor

@agraef agraef commented Aug 5, 2020

This adds an install target, so that make install will do something sensible, at least on Linux and other Unix-like systems. This will also simplify packaging.

Also added an option to link against an installed libfaust, which fixes #12 (see #14 for details).

So, to summarize, this adds:

  • 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; in that case a second STATIC_FAUST option (ON by default) allows the installed libfaust to be linked statically (setting that option to OFF will link against a shared libfaust if possible)

  • 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.

@pierreguillot
Copy link
Member

Can you describe what it does? Is it working on Windows and MacOS with XCode and Visual Studio?

@agraef
Copy link
Contributor Author

agraef commented Aug 5, 2020

See https://cmake.org/cmake/help/latest/command/install.html.

I can't tell you whether those install targets do anything useful with Xcode or Visual Studio. But they're certainly helpful from the command line, when running make install or cmake --install .

Right now make install or cmake --install . doesn't do anything, apart from generating an empty install_manifest.txt file. The rules which I added do something sensible on Linux/Unix at least, so that you can run make install or cmake --install . from the build directory to have the external installed into the lib/pd/extra/faustgen~ directory under CMAKE_INSTALL_PREFIX, which defaults to /usr/local on Unix-like systems (so that probably means macOS, too), and something like c:/Program Files/${PROJECT_NAME} on Windows.

But you can change the CMAKE_INSTALL_PREFIX, e.g., by using -DCMAKE_INSTALL_PREFIX=<dir> when running cmake, or at installation time with cmake --install . --prefix <dir>. With Unix Makefiles you can also use DESTDIR to install into a staging area. This is frequently used by package maintainers on Linux and other Unix-like systems as a quick way to populate a staging area and then construct the package contents from there, like so: make install DESTDIR=<staging-dir>. E.g., in an Arch PKGBUILD you'll be using something like: make install DESTDIR="$pkgdir". Or if you want to install into a staging directory above the build directory, you could do something like this: make install DESTDIR=../build-root. The same can be achieved using the --prefix option of cmake --install.

The lib/pd/extra/faustgen~ destination directory probably won't really be that useful on anything but Linux, but I can change that to just faustgen~ for macOS and Windows, so that you could then use something like cmake --install . --prefix ~/Library/Pd to install directly from the source on the Mac, or cmake --install . --prefix "/Program Files/Pd/extra" on Windows. If you want, you could also use that target for creating your build artifacts with Travis (well, you're also adding the package source there, but cmake --install . would get you most of the way).

Let me quickly augment the PR so that it does something more sensible on Mac and Windows. Thinking about it some more, that path should probably be a variable which you can change at configuration time.

@agraef agraef changed the title Add an install target. WIP: Add an install target. Aug 5, 2020
@agraef
Copy link
Contributor Author

agraef commented Aug 5, 2020

Ok, there you go. Better now?

@agraef
Copy link
Contributor Author

agraef commented Aug 5, 2020

@pierreguillot You probably know cmake much better than I do, so feel free to improve this. I hate cmake with a passion -- die-hard autoconf and GNU make fan here, and proud of it. ;-) Thus I don't know cmake very well, but I can quickly hack something together.

@agraef
Copy link
Contributor Author

agraef commented Aug 5, 2020

Also, please note that in my install target, I only install the *.lib files from faust/libraries and faust/libraries/old into the faustgen~/libs destination. This is in line with that the Faust compiler itself does (just that there the files get installed into /usr/share/faust, along with all the architecture files), and thus will match what we get when building the package against an installed Faust package (cf. #12). In that situation, we just want the *.lib files which are readily available in a Faust installation, whereas the other auxiliary stuff in faust/libraries is not available in a Faust installation.

…installed Faust when linking against an installed Faust version.
@agraef
Copy link
Contributor Author

agraef commented Aug 5, 2020

Ok, there we go. This also includes #14 now, so that we have everything in one place.

@agraef agraef changed the title WIP: Add an install target. Add an install target, and allow linking against an installed libfaust. Aug 5, 2020
@agraef
Copy link
Contributor Author

agraef commented Aug 6, 2020

Added another STATIC_FAUST option (enabled by default) which makes it possible to link against an installed libfaust statically. This currently only works with gcc (i.e., Linux, Un*x, Mac, and Windows with mingw, but not msvc), so this should cover all cases where you actually might want to do this.

I have tested this in all possible combinations (INSTALLED_FAUST=OFF, INSTALLED_FAUST=ON with both STATIC_FAUST=ON and OFF) on Linux, also tested the possible error conditions, they all work fine for me. The cmake code I added is pretty generic, so there's a good chance that it will work on Mac and Windows, too, but I haven't tested this. If someone else can test on Mac and Windows, please do.

@pierreguillot The latest commit also includes a work-around for the problem with LLVM 10 that you observed earlier, namely that llvm_map_components_to_libnames comes up with an empty set of libraries. In this case we also run llvm-config --libs as a fallback to get some usable linker options now. (I had to add this anyway in order to make the static installed libfaust option work; otherwise the external wouldn't load when linked statically.) I also added a warning message if llvm-config also comes up empty.

Ok, I'm done with this, I hope that everyone is happy now. CI is all good, so this is ready to go in. @pierreguillot please merge while it's hot, so that I can update my Arch PKGBUILDs. :)

@pierreguillot
Copy link
Member

Thank you! I just had a quick look and I have global remarks:

  • Perhaps the CMake function find_library() would be more appropriate?
    Something like (pseudo code):
if(INSTALLED_FAUST)
  if(STATIC_FAUST)
    if(MSVC)
      find_library(FAUST_LIBRARY libfaust.a REQUIRED...
    else()
      find_library(FAUST_LIBRARY faust.lib REQUIRED...
    endif()
  else()
    if(MSVC)
      find_library(FAUST_LIBRARY libfaust.so REQUIRED...
    else()
      find_library(FAUST_LIBRARY faust.dll REQUIRED...
    endif()
  endif()
  target_link_libraries(faustgen_tilde_project ${FAUST_LIBRARY})
endif()
  • Did you test the if(NOT llvm_libs) condition? Because if llvm_libs is empty the line above will generate an error. And using llvm-config like this assumes that it is installed in the system path, that is not the case on the CI for example. And when I tried that approach on my machine with LLVM-10, the libs were still not valid so perhaps it's just better to generate an error if there is no lib?

  • for UNIX not APPLE, there is set(INSTALL_DIR "faustgen~" CACHE STRING "Destination directory for the external"), is it at the root? If so, perhaps using ${PROJECT_SOURCE_DIR} would less intrusive? Or another solution would be to ignore all the install part for non UNIX system or APPLE.

  • find_path(FAUSTLIB all.lib PATH_SUFFIXES faust share/faust) should be used with REQUIRED, this way if there is a problem CMake generates an error.

Let me know what you think of it. I will have a better look now and do some tests.

@agraef
Copy link
Contributor Author

agraef commented Aug 6, 2020

Perhaps the CMake function find_library() would be more appropriate?

I considered that, but decided against it, because there's simply no reliable way to detect an installed libfaust across all platforms. The code I used assumes that the user knows what she's doing and has everything set up so that the Faust headers and libraries will be found without any additional magic. If that doesn't work out, then she always has the option to use the included source, which is also the default. But feel free to change this if you want to.

Did you test the if(NOT llvm_libs) condition? Because if llvm_libs is empty the line above will generate an error.

I think I do, but I'm not sure what "line above" you're referring to. Please elaborate, so that I can double-check that I did everything right.

for UNIX not APPLE, there is set(INSTALL_DIR "faustgen~" CACHE STRING "Destination directory for the external"), is it at the root?

Nope, for UNIX not APPLE, the default value is "lib/pd/extra/faustgen~". You mean the Mac/Windows case where the default value is just "faustgen~", I suppose. In any case that variable is a directory which is taken relative to CMAKE_INSTALL_PREFIX, as is customary with install targets, so it won't just clobber the root directory, if that's what you mean. I chose the "faustgen~" destination for Mac/Windows so that you can run something like cmake --install . --prefix ~/Library/Pd on the Mac, or cmake --install . --prefix "c:/Program Files/Pd/extra" on Windows and have that just work. Or maybe run something like cmake --install . --prefix staging to get the installed version under build/staging/faustgen~ from where you can copy it anywhere you want it to go.

I must admit that all this assumes that a user proficient enough to use cmake --install should also be familiar with the use of CMAKE_INSTALL_PREFIX. Otherwise he better not use cmake --install, or be prepared to live with the consequences. ;-)

find_path(FAUSTLIB all.lib PATH_SUFFIXES faust share/faust) should be used with REQUIRED, this way if there is a problem CMake generates an error.

Well, it will already give you a warning and tell you to set FAUSTLIB if the search fails. This is more forgiving, and at least lets you continue with the build and then later copy the missing lib files manually. I think that this is preferable, but feel free to change it if you disagree.

@agraef
Copy link
Contributor Author

agraef commented Aug 6, 2020

Perhaps the CMake function find_library() would be more appropriate?

Ok, I might as well give it a go. Stay tuned.

@agraef
Copy link
Contributor Author

agraef commented Aug 6, 2020

Ok, that was easier than I expected. Tested on Linux, with the Faust 2.27.2 package from Arch, works fine there. I also added libfaust.dylib for the shared lib NOT MSVC case, so that the code will hopefully work on macOS, too.

@pierreguillot
Copy link
Member

pierreguillot commented Aug 22, 2020

I think I do, but I'm not sure what "line above" you're referring to. Please elaborate, so that I can double-check that I did everything right.

You can see the error here: https://travis-ci.org/github/CICM/pd-faustgen/builds/712971074

... make --install . --prefix "c:/Program Files/Pd/extra" on Windows

So I guess the default value of INSTALL_DIR on Windows should be "Pd/extra/faustgen~"? This way, it would give "c:/Program Files/Pd/extra" by default. And I have to check on macOS.

This is more forgiving, and at least lets you continue with the build and then later copy the missing lib files manually.

Yes, but in this case, it copies the libs from a folder specific to Linux...

it seems that the MR is mainly done and tested on Linux and not the other OS, so perhaps the features should be enabled on Linux only. I'm afraid that we'll have to maintain features for Windows and macOS that never worked. And there are 3 different features in this MR, perhaps it's too much, this way, even if there is a problem with one of them we could merge the others. I guess what is important is to link against a pre-installed faust library on Linux, so let start with that. I'll try to do that before the end of August.

@agraef
Copy link
Contributor Author

agraef commented Aug 22, 2020

You can see the error here: https://travis-ci.org/github/CICM/pd-faustgen/builds/712971074

Well spotted! This is no big deal, I just need to add a check if(llvm_libs) around the list(REMOVE_ITEM llvm_libs LTO) call.

So I guess the default value of INSTALL_DIR on Windows should be "Pd/extra/faustgen~"?

IMHO that's not a workable solution. On Windows, the exe installer from vanilla indeed installs into %PROGRAMFILES%/Pd by default, but the zip archives for both Windows and Mac contain a folder Pd-version[-i386] depending on the particular version and architecture, which might be installed just anywhere where the user finds it most convenient. Good luck finding that installation anywhere on the hard disk, not even considering that a user might well keep different versions around for some reason. That's exactly why I picked an approach that makes it easy to do a staged installation and then just copy the stuff from the staging area to anywhere you want it to go.

And how many Mac and Windows users do you expect to go through all the hassle and compile faustgen~ themselves when they can readily download it here or even install it from Deken? Not many I'd say. And these few are power users like me who do want to build from source, even on Mac and Windows, and for them make install gives them all they need for getting installation done in some way. At least it's a lot better than having no install target at all and having to guess which files they need so that everything just works.

Yes, but in this case, it copies the libs from a folder specific to Linux...

I concede that this might have been a bad (and rather Linux-centric) idea, and that it's better to error out in that case. That's easy to fix, too.

it seems that the MR is mainly done and tested on Linux and not the other OS, so perhaps the features should be enabled on Linux only

Not so quick. I'm not blind to the needs of Mac and Windows users, I occasionally run these as well, and in any case most of my students do, so Mac and Windows compatibility is important for me as well. It's just that I haven't got around to testing there yet. Of course that needs to be done before releasing. But I'm confident that all this works on Mac and Windows, or can be made to with little effort.

@agraef
Copy link
Contributor Author

agraef commented Aug 23, 2020

This is a lot more robust now. As promised, I've added the if(llvm_libs) check, and the INSTALLED_FAUST option correctly detects the Faust include and library files even if they are not in the standard locations, as long as it can find the Faust library (or be told by the user where it is). Double checks are done everywhere to ensure that the files are really where they are supposed to be, otherwise a fatal error is produced, as requested.

On the other hand, I kept the default INSTALL_DIR on Mac and Windows as is, because I really think that this is the most reasonable and convenient approach on these systems for the reasons I explained.

This is also what I'm using myself in my development branch now. I tested this on Linux, Mac (using LLVM10 from MacPorts) and Windows (also using LLVM10 in Msys2), it all appears to work fine as far as I can tell. (BTW, there are some snags with pd.build producing MSVC-centric compiler options even when used with MSYS2, it would be nice if you could look into these some time. But I was able to work around these, so that I could test the entire build process including make install on Windows.) I may still do some testing with VS 2019, if I find the time.

There you go. Do with it as you please. :) Of course, I'm aware that this project has been laying dormant since 2018. So don't feel obliged to use any of this, I can understand perfectly well if you've moved on to other things. I would have done this work anyway, and being able to discuss these changes with you really helped a lot to improve these features and make them foolproof, thanks for that!

Ok, I'll stop bugging you know, and move back to my development branch where I've already taken faustgen~ much further. I have almost full feature parity with pd-faust and Grame's faustgen now, and adding the missing OSC support will be easy. All this wouldn't have been possible without your work, so thanks again for that! I'm really looking forward to use this in my master courses in the winter semester. I've made some improvements which are not backward-compatible, though, so I'll eventually rename the external, to allow both externals to be installed and used at the same time. (It goes without saying that I'll keep your credits, so that it remains clear who wrote the original version.)

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 this pull request may close these issues.

Link against existing libfaust
2 participants