-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Convert all usages of target_link_libraries to use PUBLIC/PRIVATE specifiers #81924
Convert all usages of target_link_libraries to use PUBLIC/PRIVATE specifiers #81924
Conversation
…cifiers CMake requires all usages to be consistent, and we're going to add these specifiers as part of the sanitizer work, so make them consistently used across the repo here.
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsCMake requires all usages to be consistent, and we're going to add these specifiers as part of the sanitizer work, so make them consistently used across the repo (excluding external sources) here. Split out of #74623
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mono LGTM, except I think component_base should be PRIVATE
Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
Do we also need one for
When new CMakeLists.txt files are added, what would be the best way to notify the contributor that a |
Yes, I missed that one.
We don't have automated tooling for this today, but when the work to enable AddressSanitizer gets in, the requirement will likely be enforced by CMake (as we'll add a linker flag with a specifier to many of the projects, which will cause CMake to error when someone adds a linker flag/library link without the scope specifier) |
@jkoritzinsky FYI I'm going to merge #82182 shortly which might create some conflicts with this PR. I'm adding some object libraries to mono - and they will already be using PRIVATE/PUBLIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few more in the AndroidAppBuilder and AppleAppBuilder Tasks which have a slight modification of CMakeLists.txt
name
target_link_libraries( |
runtime/src/tasks/AppleAppBuilder/Templates/CMakeLists.txt.template
Lines 69 to 85 in f9ebd42
target_link_libraries( | |
%ProjectName% | |
"-framework Foundation" | |
"-framework Security" | |
"-framework UIKit" | |
%FrameworksToLink% | |
"-lz" | |
"-lc++" | |
"-liconv" | |
%NativeLibrariesToLink% | |
) | |
if(%UseNativeAOTRuntime%) | |
target_link_libraries( | |
%ProjectName% | |
"-Wl,-u,_NativeAOT_StaticInitialization" | |
) |
Besides those, and double checking that we can ignore src/native/external/zlib
, then if the CI failures are unrelated (Could something like The type or namespace name 'ProducedBy' could not be found
be a result of a PRIVATE
scope instead of PUBLIC
?), then I think this is in good shape.
The CI failures are unrelated. There was a regression that made it into main that there are already PRs out to fix. I'll go update the AppBuilder templates. |
…ime into private-link-libraries
Do the templates actually matter? They're meant to be like examples of "user code". They're not consuming any of the cmake fragments that we have in the runtime repo. Update well ok, Jeremy updated the templates already. That's ok. it doesn't hurt to clean them up a bit |
Yeah we don't really need to update them, but I figured it was worthwhile for consistency since it was brought up. |
The templates are currently used to build custom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
All failures are known issues. |
CMake requires all usages to be consistent, and we're going to add these specifiers as part of the sanitizer work, so make them consistently used across the repo (excluding external sources) here.
Split out of #74623