kcat / openal-soft

OpenAL Soft is a software implementation of the OpenAL 3D audio API.
Other
2.22k stars 535 forks source link

Error (with fix): MSVC and Clang have compilation conflicts #839

Closed TheNachoBIT closed 5 days ago

TheNachoBIT commented 1 year ago

Hello! I want to report an issue that i was able to fix :D

This happened to me when i was trying to compile the Axmol Engine, and i had the same issue when trying to compile it outside of this engine, in other projects.

Issue: If you have MSVC and Clang installed in the same system (in this case, Clang is installed on Windows via MSYS2). The moment you want to build and compile via MSVC, It'll find and detect OSS. The problem is that its Clang's OSS and MSVC doesn't have something like that. So when you go to the compilation process, everything gets mixed (because Clang has different internal types that MSVC can't read) and ofc this results in a bunch of errors showing up.

The fix: A fix for this is to encapsulate everything that is in FindOSS.cmake with if(NOT MSVC)

My FindOSS.cmake version:

# - Find OSS includes
#
#   OSS_FOUND        - True if OSS_INCLUDE_DIR is found
#   OSS_INCLUDE_DIRS - Set when OSS_INCLUDE_DIR is found
#   OSS_LIBRARIES    - Set when OSS_LIBRARY is found
#
#   OSS_INCLUDE_DIR - where to find sys/soundcard.h, etc.
#   OSS_LIBRARY     - where to find libossaudio (optional).
#
if(NOT MSVC)
    find_path(OSS_INCLUDE_DIR
              NAMES sys/soundcard.h
              DOC "The OSS include directory"
    )

    find_library(OSS_LIBRARY
                 NAMES ossaudio
                 DOC "Optional OSS library"
    )

    include(FindPackageHandleStandardArgs)
    find_package_handle_standard_args(OSS  REQUIRED_VARS OSS_INCLUDE_DIR)

    if(OSS_FOUND)
        set(OSS_INCLUDE_DIRS ${OSS_INCLUDE_DIR})
        if(OSS_LIBRARY)
            set(OSS_LIBRARIES ${OSS_LIBRARY})
        else()
            unset(OSS_LIBRARIES)
        endif()
    endif()

    mark_as_advanced(OSS_INCLUDE_DIR OSS_LIBRARY)
endif()

I'm not making a PR because this is probably a duck-tape solution, but i hope this helps :)

kcat commented 1 year ago

Odd that MSVC would find and try to use headers from MSYS2/Clang. But yeah, this wouldn't be the proper way to do it. Ideally it shouldn't try to look for or use backends that a compiler isn't designed for in the first place.

TheNachoBIT commented 1 year ago

Odd that MSVC would find and try to use headers from MSYS2/Clang. But yeah, this wouldn't be the proper way to do it. Ideally it shouldn't try to look for or use backends that a compiler isn't designed for in the first place.

I think this is a CMake issue because it says in the build, if i use the current version that doesn't have the if(NOT MSVC):

- Found OSS: /path/to/msys64/usr/include, which is the path to clang includes.

kcat commented 1 year ago

That seems like a bigger issue, that it's finding MSYS2 as a valid header location in general despite using a non-MSYS2 compiler. That likely means other header checks will erroneously succeed too if MSYS2 has them, causing other things to get enabled that shouldn't be for MSVC.

TheNachoBIT commented 1 year ago

That seems like a bigger issue, that it's finding MSYS2 as a valid header location in general despite using a non-MSYS2 compiler. That likely means other header checks will erroneously succeed too if MSYS2 has them, causing other things to get enabled that shouldn't be for MSVC.

I don't think so because so far that's the only issue, the rest of the headers are fine and all point to MSVC includes instead of clang includes.

I think this is being triggered because OpenAL doesn't know that OSS doesn't exist in MSVC, so it tries to look it up in the global path regardless, where clang is also global aswell.

kcat commented 1 year ago

OpenAL tries not to assume anything, since you never know if a backend may become available for unexpected reasons. So it looks in the paths for the compiler as detected by CMake. This may be an instance where an assumption is needed, though.

kcat commented 1 year ago

This should be fixed with commit 3e4a2a151860c04fb0ebb971343a6f05e0c3c08a. It won't check for the ALSA, OSS, Solaris, or SoundIO backends on Windows systems.