xiph / vorbis

Reference implementation of the Ogg Vorbis audio format.
BSD 3-Clause "New" or "Revised" License
474 stars 193 forks source link

Add Ogg::ogg alias to main library target? #69

Closed eliasdaler closed 4 years ago

eliasdaler commented 4 years ago

Hello. I'm building vobris as a part of my project (calling add_subdirectory on this repo) and so I don't use FindOgg.cmake for finding vorbis. Can you please add "Ogg::ogg" target, so I can link with it instead of "vobris" and "vorbisfile"?

eliasdaler commented 4 years ago

Actually, after looking into it more, it seems like the issue is that vorbis expects to find already built libogg and doesn't allow you to do something like this:

add_subdirectory(ogg) # xiph/ogg
add_subdirectory(vorbis) # this repo

Is it possible to somehow make find_package(Ogg REQUIRED) in main CMakeLists.txt optional?

evpobr commented 4 years ago

If FindOgg.cmake is not found, CMake with switch to package config mode. Latest Ogg provides config file, it is generated in build directory (usually build). You can use <Package-Name>_Dir to set config directory, in your case it is Ogg_Dir.

Try:

cmake .. -DOgg_Dir=<path-to-ogg-directory>/<build-directory>
DatCaptainHorse commented 4 years ago

I've been trying to get this messy OGG-Vorbis CMake conga working for multiple hours now, it's 3am and I will voice my opinions in semi-contained anger.

[FindOgg.cmake - line:85] Do NOT create a OGG library in Vorbis! WHY. It's up to OGG's CMakeLists to make sure Ogg::ogg is available and usable. Do you go to a SHIPYARD to build a CAR?

[FindOgg.cmake - line:56] Looking for a system library, okay. But if you are trying to build using add_subdirectory(), enjoy your torment! WHY do you need OGG immediately? You COULD ATLEAST add an if/else statement in the CMakeLists checking if we are building as subdirectory if for some reason having OGG there right now is so important.

Easy fix? Delete FindOgg.cmake and set Ogg_DIR pointing to where OGG is being built to, but hey that opens another can of worms in OGG's side :-)

I could also list issues from OGG's CMakeLists side but atleast I am able differentiate it from Vorbis and not put in in the issue here. Spoiler: Exports are unusable

If I feel particularly codey after a good nights sleep, I may try to fix the CMake conga or in worse case rewrite it. I'm tired of having to do set(OGG_HACK_DIR) things to make this work.

evpobr commented 4 years ago

Find modules are correct (more or less), @CaptainHorse.

The truth is that Ogg and Vorbis СMake projects are not designed to be used as subprojects. Sorry for this.

But the good news is that the fix is trivial. I guess we need to use ALIAS libraries:

Can you test https://gitlab.xiph.org/evpobr/ogg/-/commit/163c5d6aab4c6a25df110e0f80f6e7dc6a0360be.

This will expose Ogg::ogg target if you add Ogg CMake project with add_subdirectory.

If it works i will create pull requests for Ogg and Vorbis.

evpobr commented 4 years ago

And Vorbis: https://gitlab.xiph.org/evpobr/vorbis/-/commit/e1b2fabbe3624c0d0dd3226f2af48f5ef582744b

evpobr commented 4 years ago

So i've tested it.

Directory layout:

cmake_test/
cmake_test/ogg
cmake_test/vorbis
cmake_minimum_required(VERSION 3.13..3.18)

project(cmake_test VERSION 0.1.0)

add_subdirectory(ogg)
add_subdirectory(vorbis)

add_executable(cmake_test cmake_test.c)
target_link_libraries(cmake_test PRIVATE Vorbis::vorbisenc)

cmake_test.c:

#include <vorbis/vorbisenc.h>

#include <stdlib.h>
#include <stdio.h>

int main(void)
{
    vorbis_info vi = {0};
    vorbis_encode_setup_init(&vi);

    return 0;
}

Output:

[build] [2/2 100% :: 0.874] cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=CMakeFiles\cmake_test.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100183~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100183~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1427~1.291\bin\Hostx64\x64\link.exe  CMakeFiles\cmake_test.dir\cmake_test.c.obj  /out:cmake_test.exe /implib:cmake_test.lib /pdb:cmake_test.pdb /version:0.0 /machine:x64 /debug /INCREMENTAL /subsystem:console  vorbis\lib\vorbisenc.lib  vorbis\lib\vorbis.lib  ogg\ogg.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."

But Ogg dependency is still problematic. The only solution i've found is to set Ogg_DIR to Ogg's build directory:

cmake .. -DOgg_DIR="<cmake-test-directory>/ogg/build"

Now everything works for me as expected.

DatCaptainHorse commented 4 years ago

Hail Omnissiah, that almost works

But Ogg dependency is still problematic.

After deleting line 65 find_package(Ogg REQUIRED) in the main CMakeLists things work. Removing REQUIRED and using some checks whether Ogg was found can b used if the line is super necessary.

Thank you for tackling this issue, I very much hope this gets to the main branch after some fixes to the Ogg dependency.

evpobr commented 4 years ago

I'm pretty sure you are using add_subdirectory incorrectly. This function is designed to organize your project into subprojects. In this case, everything looks logical and it becomes clear why find_package does not work.

If you try a normal package manager like Vcpkg, you will see that it downloads and build dependencies, installs them in a specific location, and then configures search paths so that find_package call in your project finds them.

But OK, fix for you is simple. Remove REQUIRED is not good, because Ogg dependency is, hmm, required. I guess the correct way is to wrap find_package(Ogg) and find_dependency(Ogg) calls in:

if(TARGET Ogg::ogg)
    ...
endif()

Check it please if you have time.

evpobr commented 4 years ago

https://gitlab.xiph.org/xiph/ogg/-/merge_requests/6

https://gitlab.xiph.org/xiph/vorbis/-/merge_requests/14

rillian commented 4 years ago

I merged @evpobr's changes, if that makes it easier to test.

DatCaptainHorse commented 4 years ago

These changes are working 👍

Yeah as @evpobr thought I may not be using add_subdirectory in the most correct way, but I've found it to be the most simple and minimal setup for anyone cloning and setting up my engine project.

Currently working on application using the engine and it's add_subdirectories basically going like this:

Setting up clean project is as easy as cloning it and doing git submodule update --init --remote --recursive (unless you want to build Python scripting module, CPython does not make anything easy, no CMake support, source and config files spread everywhere.. It's the only dependency I wish to keep pre-built blobs of due to it being a pain to compile with correct settings..)

eliasdaler commented 4 years ago

add_subdirectory is not only used for adding your projects into CMake build. See FetchContent` documentation - add_subdirectory is done to download and configure all dependencies at configuration time without any pre-build step. Also thanks for the changes - it looks like this will solve the issue, I'll test it soon.

rillian commented 4 years ago

Everyone happy with the code now?

DatCaptainHorse commented 4 years ago

Yes

rillian commented 4 years ago

Ok, great. Treating this as resolved.