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

Use add_link_options and target_link_options in cmake #92844

Merged
merged 17 commits into from
Oct 17, 2023

Conversation

jtschuster
Copy link
Member

Fixes #92843

Replaces add_linker_flag with add_linker_options and target_linker_options, where possible.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 30, 2023
@ghost ghost assigned jtschuster Sep 30, 2023
@jkoritzinsky
Copy link
Member

There's a slight UX difference between the add_linker_flag function we have and add_link_options. Our function takes the config as a second parameter. add_link_options doesn't, so we'd need to use generator expressions.

Also the way you specify the -Wl, flags is different as well. Here's a table that shows how to go from the add_linker_flag way to the add_link_options way:

add_linker_flag add_link_options Useful doc link
add_linker_flag(foo DEBUG) add_link_options($<$<CONFIG:Debug>:foo>) https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
add_linker_flag(-Wl,-z,notext) add_link_options("LINKER:-z,notext") https://cmake.org/cmake/help/latest/command/add_link_options.html#handling-compiler-driver-differences

@jkoritzinsky
Copy link
Member

The remaining failures on Windows look like they're in the RC-only targets. Right now these targets remove linker flags that they are not compatible with. Instead, we can update the add_link_options for these flags to exclude RC-only targets by using the generator expression: $<$<NOT:$<LINK_LANGUAGE:RC>>:flag>. Looks like we need this for the /LTCG flag and the /CETCOMPAT flag.

@jkoritzinsky jkoritzinsky added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 2, 2023
@ghost
Copy link

ghost commented Oct 2, 2023

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #92843

Replaces add_linker_flag with add_linker_options and target_linker_options, where possible.

Author: jtschuster
Assignees: jtschuster
Labels:

area-Infrastructure

Milestone: -

@jtschuster jtschuster changed the title Use add_linker_options and target_linker_options in cmake Use add_link_options and target_link_options in cmake Oct 13, 2023
@jtschuster
Copy link
Member Author

Failure looks like #93527

@jtschuster jtschuster merged commit 3086d8a into dotnet:main Oct 17, 2023
201 of 206 checks passed
@lewing
Copy link
Member

lewing commented Oct 18, 2023

Looks like this broke the official build

2023-10-18T11:44:14.3256632Z   CMake Error at pgosupport.cmake:21 (target_link_options):
2023-10-18T11:44:14.3262066Z     target_link_options called with invalid arguments
2023-10-18T11:44:14.3262652Z   Call Stack (most recent call first):
2023-10-18T11:44:14.3263392Z     jit/CMakeLists.txt:626 (add_pgo)
2023-10-18T11:44:14.3264832Z   
2023-10-18T11:44:14.3266461Z   
2023-10-18T11:44:14.3268303Z   CMake Error at pgosupport.cmake:22 (target_link_options):
2023-10-18T11:44:14.3269877Z     target_link_options called with invalid arguments
2023-10-18T11:44:14.3271707Z   Call Stack (most recent call first):
2023-10-18T11:44:14.3273617Z     jit/CMakeLists.txt:626 (add_pgo)
2023-10-18T11:44:14.3274400Z   
2023-10-18T11:44:14.3275848Z   
2023-10-18T11:44:15.0339628Z   Read file version from native version header at 'D:/a/_work/1/s/artifacts/obj/_version.h'.
2023-10-18T11:44:15.0436800Z   CMake Error at pgosupport.cmake:21 (target_link_options):
2023-10-18T11:44:15.0437813Z     target_link_options called with invalid arguments
2023-10-18T11:44:15.0438984Z   Call Stack (most recent call first):
2023-10-18T11:44:15.0441399Z     dlls/mscoree/coreclr/CMakeLists.txt:215 (add_pgo)
2023-10-18T11:44:15.0443545Z   
2023-10-18T11:44:15.0445423Z   
2023-10-18T11:44:15.0447812Z   CMake Error at pgosupport.cmake:22 (target_link_options):
2023-10-18T11:44:15.0449982Z     target_link_options called with invalid arguments
2023-10-18T11:44:15.0451924Z   Call Stack (most recent call first):
2023-10-18T11:44:15.0454038Z     dlls/mscoree/coreclr/CMakeLists.txt:215 (add_pgo)
2023-10-18T11:44:15.0456001Z   
2023-10-18T11:44:15.0457863Z   
2023-10-18T11:44:15.0841406Z   -- Configuring incomplete, errors occurred!
2023-10-18T11:44:15.1132740Z ##[error]BUILD: Error: failed to generate native component build project

akoeplinger added a commit that referenced this pull request Oct 18, 2023
This got broken in #92844, it was missing the scope keyword.
jkotas added a commit to jkotas/runtime that referenced this pull request Oct 23, 2023
jkotas added a commit that referenced this pull request Oct 23, 2023
…" (#93838)

* Revert "Fix target_link_options in pgosupport.cmake (#93670)"

This reverts commit fa0ba15.

* Revert "Use `add_link_options` and `target_link_options` in cmake (#92844)"

This reverts commit 3086d8a.
@janvorli
Copy link
Member

@jtschuster, @jkoritzinsky, @jkotas it also accidentally enabled CETCOMPAT for executables (e.g. corerun.exe).

@ghost ghost locked as resolved and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the add_linker_flag function and instead use target_link_options
4 participants