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.

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

Conversation

ntonnaett
Copy link

@ntonnaett ntonnaett commented Oct 23, 2024

Hive failed to build for me on Linux. So I'm working on making the build process more convenient for Linux users. I changed to the recommended model for using dependencies that utilizes FetchContent but first tries to find dependencies with find_package. I did this only for common libraries (fmt, googletest).

I'm not sure how the header only usage of fmt was supposed to work. I added the FMT_HEADER_ONLY variable and removed the linking step that shouldn't be necessary.

I'm wondering if the find_package step should be disabled on Mac and Windows.

I'm opening this PR now so I can get input early on how to make these changes without disrupting your workflow.

@ntonnaett ntonnaett marked this pull request as draft October 23, 2024 07:56
@christophe-calmejane
Copy link
Contributor

Hi, thanks a lot for that, I'll need to take a look when I get a chance. My idea for linux was to create an AppImage (through Docker) in order to guarantee it will compile correctly everywhere. Currently there is already a docker setup which I use to validate compilation, but I'd be happy to take advantage of new CMake features.

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.

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants