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

fix linker flags in cmake (find_package based) generators #8740

Merged
merged 4 commits into from Apr 6, 2021

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Mar 31, 2021

Changelog: Bugfix: Fix linker flags in cmake (find_package based) generators.
Docs: Omit

Fix: #8703

#tags: slow

Comment on lines 238 to 240
self.sharedlinkflags_list = join_flags(";", ["-{}".format(f)
for f in cpp_info.sharedlinkflags])
self.exelinkflags_list = join_flags(";", ["-{}".format(f) for f in cpp_info.exelinkflags])
Copy link

Choose a reason for hiding this comment

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

Could this break flags that already start with a - or --, for non-MSVC linkers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch, I will verify that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Keeping flags that already start with "-" in 4ebe404

@memsharded memsharded requested review from SSE4 and danimtb April 4, 2021 20:56
@memsharded memsharded self-assigned this Apr 4, 2021
files = gen.content
d = files["mypkgTarget-release.cmake"]
assert "$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,EXECUTABLE>:-/OPT:NOICF>" in d
assert "$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,MODULE_LIBRARY>:-/NODEFAULTLIB;-/OTHERFLAG>" in d
Copy link
Contributor

Choose a reason for hiding this comment

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

so, is it really expected to pass -/OPT with both dash and slash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently it worked, but you are right, better double check, I'll check it.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that syntax looks a bit weird to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it was working, it didn't raise an error, but just a warning. Dropped the extra /, please check.

conan/tools/cmake/cmakedeps.py Outdated Show resolved Hide resolved
conans/client/generators/cmake.py Outdated Show resolved Hide resolved
files = gen.content
d = files["mypkgTarget-release.cmake"]
assert "$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,EXECUTABLE>:-/OPT:NOICF>" in d
assert "$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,MODULE_LIBRARY>:-/NODEFAULTLIB;-/OTHERFLAG>" in d
Copy link
Member

Choose a reason for hiding this comment

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

yeah, that syntax looks a bit weird to me

@memsharded memsharded requested review from SSE4 and danimtb April 5, 2021 18:20
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.

None yet

4 participants