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

Set target_link_libraries visibility to PRIVATE for linking android log #2815

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

itacud95
Copy link
Contributor

@itacud95 itacud95 commented Feb 19, 2024

Description

Set visibility for target_link_libraries to PRIVATE when linking in log for Android.

INTERFACE should be used on dependencies from other than our source files.
PRIVATE should be used the include happens in our source files.

In this case (src/catch2/internal/catch_debug_console.cpp:20) the include is from our source file.

I encountered a issue with this when building Catch2 for Android in combination with BUILD_SHARED_LIBS. Changing the visibility to PRIVATE fixes the issue.

Reproduction:

cmake -Bbuild -DANDROID=on -DBUILD_SHARED_LIBS=on -DCMAKE_TOOLCHAIN_FILE=$ANDROID_NDK_ROOT/build/cmake/android.toolchain.cmake -DANDROID_ABI=x86_64 && cmake --build build

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (28c2f0b) 91.36% compared to head (7ea6531) 91.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #2815      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.02%     
==========================================
  Files         197      197              
  Lines        8289     8289              
==========================================
- Hits         7573     7571       -2     
- Misses        716      718       +2     

@horenmar horenmar added the Building and Packaging Issues affecting build/packaging scripts and utilities label Feb 19, 2024
@horenmar
Copy link
Member

Thanks, this is indeed correct. I wonder how it came to be an INTERFACE link in the first place.

@horenmar horenmar merged commit 024cfb3 into catchorg:devel Feb 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building and Packaging Issues affecting build/packaging scripts and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants