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

[WIP] Make Linux compilation more comfortable #157

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[WIP] Make Linux compilation more comfortable #157

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
18 changes: 16 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,17 @@ cu_setup_project_version_variables(${LA_AVDECC_VERSION})
# Set minimum OSX/iOS version
cu_setup_apple_minimum_versions(MACOS 10.13)

include(FetchContent)

# Include fmtlib
if(ENABLE_AVDECC_USE_FMTLIB)
add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/externals/3rdparty/fmtlib EXCLUDE_FROM_ALL)
FetchContent_Declare(
fmt
GIT_REPOSITORY https://github.com/fmtlib/fmt.git
GIT_TAG 0c9fce2ffefecfdce794e1859584e25877b7b592 # release-11.0.2
FIND_PACKAGE_ARGS NAMES fmt
)
FetchContent_MakeAvailable(fmt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the documentation, when using FIND_PACKAGE_ARGS, CMake will first try to find the library using find_package which is not something desired. If a newer (or older) version is installed somewhere on the system, it will be used instead of the one explicitly defined and validated by the project. Not a controlled environment.

Copy link
Author

Choose a reason for hiding this comment

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

You can still disable this behavior with FETCHCONTENT_TRY_FIND_PACKAGE_MODE in your controlled environment. But I don't have your controlled environment and avdecc just doesn't compile for me because fmt is outdated. When I want to get Hive into Fedora I'm not allowed to bundle every dependency. Dependencies -- even static libraries -- need to be tracked and there is always only one version in the repositories.

I think the best approach to get what most people expect is to set FETCHCONTENT_TRY_FIND_PACKAGE_MODE to NEVER on macOS and Windows. On Linux everybody expects build systems to get their dependencies from the system. If you build an application with Flatpak it would automatically use FetchContent because the dependencies are missing. So it would work out-of-the-box.

endif()

# Include json
Expand Down Expand Up @@ -224,7 +232,13 @@ if(BUILD_AVDECC_TESTS AND NOT VS_USE_CLANG)
set(BUILD_GMOCK ON CACHE BOOL "Build the googlemock subproject" FORCE)
set(INSTALL_GTEST OFF CACHE BOOL "Disable installation of googletest. (Projects embedding googletest may want to turn this OFF.)" FORCE)
# Include gtest framework
add_subdirectory(externals/3rdparty/gtest)
FetchContent_Declare(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG 703bd9caab50b139428cea1aaff9974ebee5742e # release-1.10.0
FIND_PACKAGE_ARGS NAMES GTest
)
FetchContent_MakeAvailable(googletest)
# Include our unit tests
add_subdirectory(tests)
endif()
Expand Down
3 changes: 1 addition & 2 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,7 @@ if(BUILD_AVDECC_LIB_SHARED_CXX)

# Setup libfmt
if(ENABLE_AVDECC_USE_FMTLIB)
target_compile_options(${PROJECT_NAME}_cxx PRIVATE -DHAVE_FMT)
target_link_libraries(${PROJECT_NAME}_cxx PRIVATE fmt-header-only)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how that change would work better than using the fmt author provided header-only target. Linking with it guarantees everything is correctly setup by the author. I actually don't see how it can work without specifying the location of the header file. I suspect it would fall in a working state because cmake automatically adds the fetched content as a "-I" path. But if the author then decides to change the internal define (ie. FMT_HEADER_ONLY) or add a new one, it will not be passed to the compilation units if not using the defined target.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, it failed to find fmt-header-only and somehow I didn't find it upstream either. I probably was too tired. It looks like I just have to change fmt-header-only to fmt::fmt-header-only. I'll change that.

target_compile_options(${PROJECT_NAME}_cxx PRIVATE -DHAVE_FMT -DFMT_HEADER_ONLY)
endif()

# Setup json
Expand Down
3 changes: 1 addition & 2 deletions src/controller/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ if(BUILD_AVDECC_LIB_SHARED_CXX)

# Setup libfmt
if(ENABLE_AVDECC_USE_FMTLIB)
target_compile_options(${PROJECT_NAME}_cxx PRIVATE -DHAVE_FMT)
target_link_libraries(${PROJECT_NAME}_cxx PRIVATE fmt-header-only)
target_compile_options(${PROJECT_NAME}_cxx PRIVATE -DHAVE_FMT -DFMT_HEADER_ONLY)
endif()

# Setup json
Expand Down