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.

Sign up for GitHub

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

Enable djinterop on Windows CI builds #3155

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Enable djinterop on Windows CI builds #3155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ endif()
#######################################################################

set(CMAKE_CXX_STANDARD 17)
if(MSVC)
# Ensure MSVC populates __cplusplus correctly.
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zc:__cplusplus")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for the current version of libdjinterop (which looks only at __cplusplus and doesn't use any _MSVC_LANG hacks), but may be considered a good idea to enable anyway.

FWIW, if used on an older version of MSVC, this prints an "unrecognised option" message and carries on anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it suffice to add this to the libdjinterop CMakeLists.txt? Or is this required for libdjinterop includes? If so, will building Mixxx still work if an older MSVC version is used that doesn't support the flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is already set in libdjinterop's own CMakeLists.txt file, but it is indeed also required to use libdjinterop's public includes, which is why it's required in Mixxx in order to use libdjinterop.

The /Zc:__cplusplus flag is supported from VS 2017 onwards, which appears to be the minimum-supported version to build Mixxx (according to the wiki). That was my reasoning for adding the flag for the whole MSVC build - hope this is ok.

endif()

# Speed up builds on HDDs
if(GNU_GCC OR LLVM_CLANG)
Expand Down Expand Up @@ -1489,25 +1493,41 @@ if(ENGINEPRIME)
target_link_libraries(mixxx-lib PUBLIC DjInterop::DjInterop)
else()
# Fetch djinterop sources from GitHub and build them statically.
message(STATUS "Linking statically to libdjinterop fetched from GitHub")

# On MacOS, Mixxx does not use system SQLite, so we will use libdjinterop's
# embedded SQLite in such a case.
if (APPLE)
message(STATUS "Building libdjinterop sources (with embedded SQLite) fetched from GitHub")
set(DJINTEROP_SYSTEM_SQLITE OFF)
else()
message(STATUS "Building libdjinterop sources (with system SQLite) fetched from GitHub")
set(DJINTEROP_SYSTEM_SQLITE ON)
endif()

set(DJINTEROP_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/lib/libdjinterop-install")
set(DJINTEROP_LIBRARY "lib/${CMAKE_STATIC_LIBRARY_PREFIX}djinterop${CMAKE_STATIC_LIBRARY_SUFFIX}")
Copy link
Contributor Author

@mr-smidge mr-smidge Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying lib here (rather than ${CMAKE_INSTALL_LIBDIR}) follows the pattern used for keyfinder, and should resolve the issue seen by @uklotzde here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable since CMAKE_INSTALL_LIBDIR refers to the standard system location while the local builds use just "lib".

ExternalProject_Add(libdjinterop
GIT_REPOSITORY https://github.com/xsco/libdjinterop.git
GIT_TAG tags/0.13.0
GIT_TAG tags/0.14.3
GIT_SHALLOW TRUE
INSTALL_DIR ${DJINTEROP_INSTALL_DIR}
CMAKE_ARGS "-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}" -DCMAKE_INSTALL_PREFIX:PATH=<INSTALL_DIR>
CMAKE_ARGS
-DBUILD_SHARED_LIBS=OFF
-DCMAKE_SKIP_INSTALL_ALL_DEPENDENCY=ON
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-DCMAKE_INSTALL_PREFIX:PATH=<INSTALL_DIR>
-DCMAKE_PREFIX_PATH:PATH=${CMAKE_PREFIX_PATH}
-DSYSTEM_SQLITE=${DJINTEROP_SYSTEM_SQLITE}
BUILD_BYPRODUCTS <INSTALL_DIR>/${DJINTEROP_LIBRARY}
EXCLUDE_FROM_ALL TRUE
)

# Assemble a library based on the external project.
add_library(mixxx-libdjinterop STATIC IMPORTED)
add_dependencies(mixxx-libdjinterop libdjinterop)
set(DJINTEROP_INCLUDE_DIR "${DJINTEROP_INSTALL_DIR}/include")
set(DJINTEROP_LIBRARY "${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}djinterop${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(DJINTEROP_LIBRARY_DIR "${DJINTEROP_INSTALL_DIR}/${DJINTEROP_LIBRARY}")
set_target_properties(mixxx-libdjinterop PROPERTIES IMPORTED_LOCATION "${DJINTEROP_LIBRARY_DIR}")
set(DJINTEROP_LIBRARY_PATH "${DJINTEROP_INSTALL_DIR}/${DJINTEROP_LIBRARY}")
set_target_properties(mixxx-libdjinterop PROPERTIES IMPORTED_LOCATION "${DJINTEROP_LIBRARY_PATH}")
target_include_directories(mixxx-lib PUBLIC ${DJINTEROP_INCLUDE_DIR})
target_link_libraries(mixxx-lib PUBLIC mixxx-libdjinterop)

Expand Down
1 change: 1 addition & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ for:
-DBATTERY=ON
-DBROADCAST=ON
-DBULK=ON
-DENGINEPRIME=ON
-DHID=ON
-DHSS1394=ON
-DKEYFINDER=OFF
Expand Down