ValveSoftware / vogl

OpenGL capture / playback debugger.
MIT License
1.42k stars 126 forks source link

jpeg-turbo detection doesn't properly handle not finding it #191

Open mstewartgallus opened 9 years ago

mstewartgallus commented 9 years ago

The code here isn't good. It should error out if it can't find jpeg-turbo and not just fill in some randomness and result in a link failure halfway through building Vogl.

    else()
        if (BUILD_X64)
            set(BITS_STRING "x64")
        else()
            set(BITS_STRING "x86")
        endif()
        set(LibJpegTurbo_INCLUDE "${CMAKE_PREFIX_PATH}/libjpeg-turbo-2.1.3/include" PARENT_SCOPE)
        set(LibJpegTurbo_LIBRARY "${CMAKE_PREFIX_PATH}/libjpeg-turbo-2.1.3/lib_${BITS_STRING}/turbojpeg.lib" PARENT_SCOPE)
    endif()
pwaller commented 9 years ago

I just hit this. Needed to install more dependencies, would have been nice if cmake would have failed instead of the make.

vogl/vogl_build/release64$ make
[ 27%] Built target voglcore
[ 27%] Built target voglgen
[ 28%] Built target voglgen_make_inc
[ 33%] Built target backtracevogl
[ 50%] Built target voglcommon
[ 54%] Built target vogl
[ 54%] Built target voglbench
make[2]: *** No rule to make target `/libjpeg-turbo-2.1.3/lib_x64/turbojpeg.lib', needed by `../libvogltrace64.so'. Stop.
make[1]: *** [src/vogltrace/CMakeFiles/vogltrace.dir/all] Error 2
kingtaurus commented 9 years ago

@sstewartgallus It looks like the code that you quote is from src/build_options.cmake.

I'll quote the full function here:

function(require_libjpegturbo)
    find_library(LibJpegTurbo_LIBRARY
        NAMES libturbojpeg.so libturbojpeg.so.0 libturbojpeg.dylib
        PATHS /opt/libjpeg-turbo/lib 
    )

    # On platforms that find this, the include files will have also been installed to the system
    # so we don't need extra include dirs.
    if (LibJpegTurbo_LIBRARY)
        set(LibJpegTurbo_INCLUDE "" PARENT_SCOPE)

    # On Darwin we assume a Homebrew install
    elseif (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
        set(LibJpegTurbo_INCLUDE "/usr/local/opt/jpeg-turbo/include"            PARENT_SCOPE)
        set(LibJpegTurbo_LIBRARY "/usr/local/opt/jpeg-turbo/lib/libturbojpeg.a" PARENT_SCOPE)

    else()
        if (BUILD_X64)
            set(BITS_STRING "x64")
        else()
            set(BITS_STRING "x86")
        endif()
        set(LibJpegTurbo_INCLUDE "${CMAKE_PREFIX_PATH}/libjpeg-turbo-2.1.3/include" PARENT_SCOPE)
        set(LibJpegTurbo_LIBRARY "${CMAKE_PREFIX_PATH}/libjpeg-turbo-2.1.3/lib_${BITS_STRING}/turbojpeg.lib" PARENT_SCOPE)
    endif()
endfunction()

CMake looks in /opt/libjpeg-turbo/lib in addition CMake associated paths (for the turbojpeg library). I believe the else section that you quote is a residual of the original build procedure (and is unlikely to find turbojpeg). I agree that it would be better to replace the else() with failing out with an error.

So, perhaps the else() above becomes:

message(FATAL_ERROR "Unable to find libturbojpeg")

In addition, the search for the header, /usr/include/turbojpeg.h should be made explicit (especially if header is placed in a path that is not being searched). The configuration process would succeed, but the build process would fail at the attempted to find turbojpeg.h during compilation of src/vogltrace/vogl_intercept.cpp.

# On platforms that find this, the include files will have also been installed to the system
# so we don't need extra include dirs.
if (LibJpegTurbo_LIBRARY)
   set(LibJpegTurbo_INCLUDE "" PARENT_SCOPE)

to something like (I haven't tested below, but I believe this should work):

if (LibJpegTurbo_LIBRARY)
   find_path(LIBJPEGTURBO_INCLUDE_DIR
                   NAME turbojpeg.h
                   PATHS /opt/libjpeg-turbo/inc)
   IF(NOT LIBJPEGTURBO_INCLUDE_DIR)
     message(FATAL_ERROR "Unable to find turbojpeg.h")
   ENDIF()
   include_directories(${LIBJPEGTURBO_INCLUDE_DIR})

I would recommend an additional set of checks to see the library found by CMake has the appropriate set of symbols defined and the version of the found library is correct.

EDIT: removed an extra ENDIF()