conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.11k stars 964 forks source link

[bug] CMakeDeps/CMakeToolchain generators put quotation marks around exelinkflags in linker call if they contain spaces #16634

Open PhilippFinke1988 opened 2 months ago

PhilippFinke1988 commented 2 months ago

Describe the bug

Environment details

Description

We have quite a bunch of Conan packages containing static libraries that have to be linked as "whole-archive" in GCC (the reason why we need this is out of scope for this issue). This is done by surrounding the path to the library with -Wl,--whole-archive and -Wl,--no-whole-archive ^1 in the linker call. We achieve this by adding an entry to cpp_info.exelinkflags in method package_info() so consumers will use the correct linker flags automatically. The following code block shows an example of this (please mind the spaces in the string).

def package_info(self):
    ....
    self.cpp_info.exelinkflags += ["-Wl,--whole-archive path/to/libfoo.a -Wl,--no-whole-archive"]
    ...

This results in a linker call in the consumer that looks something like this:

conan: path/to/cmake/data/bin/cmake -E cmake_link_script CMakeFiles/example.dir/link.txt --verbose=1
conan: path/to/gcc-arm-none-eabi-10.3-2022.10/bin/arm-none-eabi-g++ CMakeFiles/example.dir/src/main.cpp.obj -o bin/example -Wl,--whole-archive path/to/libfoo.a -Wl,--no-whole-archive

This worked pretty well with the old and now deprecated cmake helper generator. In preparation of switching to Conan 2, we replaced the cmake helper generator with the new CMakeDeps and CMakeToolchain generators. But now, in the linker call, quotation marks are put around the exelinkflags which leads to a linker error.

conan: path/to/cmake/data/bin/cmake -E cmake_link_script CMakeFiles/example.dir/link.txt --verbose=1
conan: path/to/gcc-arm-none-eabi-10.3-2022.10/bin/arm-none-eabi-g++ CMakeFiles/example.dir/src/main.cpp.obj -o bin/example "-Wl,--whole-archive path/to/libfoo.a -Wl,--no-whole-archive"
stderr: path/to/gcc-arm-none-eabi-10.3-2022.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: unrecognized option '--whole-archive path/to/libfoo.a -Wl'

It seems that this is caused by the spaces in the exelinkflags as flags that contain no spaces work just fine.

I tried to solve this be refactoring the exelinkflags to

def package_info(self):
    ....
    self.cpp_info.exelinkflags += ["-Wl,--whole-archive", "path/to/libfoo.a", "-Wl,--no-whole-archive"]
    ...

which works in some cases, but most times it reorders the linker flags to something like this

conan: path/to/cmake/data/bin/cmake -E cmake_link_script CMakeFiles/example.dir/link.txt --verbose=1
conan: path/to/gcc-arm-none-eabi-10.3-2022.10/bin/arm-none-eabi-g++ CMakeFiles/example.dir/src/main.cpp.obj -o bin/example -Wl,--whole-archive -Wl,--no-whole-archive path/to/libfoo.a

I think this is a bug in Conan (at least in 1.X) and I hope you can fix this.

How to reproduce it

Prerequisites:

Steps to reproduce:

  1. Unzip foo.zip
  2. Change into package root of foo.
  3. Call conan create . demo/develop
memsharded commented 2 months ago

Hi @PhilippFinke1988

Thanks for your report.

The current quotation for linker flags in CMakeDeps comes from: https://github.com/conan-io/conan/issues/9936, which reported not having quotes as a bug.

In https://github.com/conan-io/conan/pull/9980 a new test was added that uses spaces in the linker flags, and does a real compilation using CMakeDeps. Apparently this test is passing without issues. I have done a quick check and the quoting seems to happen irrespective of the spaces.

Also this change has been there since Conan 1.40.1, like 3 years ago, it is a little bit surprising to hear that it is failing. Maybe there is something else that is failing? What cmake version are you using?

PhilippFinke1988 commented 2 months ago

Hi @memsharded,

thanks for the fast reply.

I'm using CMake 3.27.2. I did not change the CMake version while migrating from the cmake helper to CMakeDeps. In ticket #9936 I cannot find any reference to adding quotation marks to linker flags, but only an issue that linker flags have been appended to the incorrect list in CMake.

memsharded commented 2 months ago

The report in https://github.com/conan-io/conan/issues/9936#issue-1043572776 happened when quotes were not defined for flags with spaces that are actually the same flag, because CMake was apparently converting it to a list of separate flags -z norel without quotes becomes -z;norel as if -z and norel were different flags, and then some flags processing can alter that and separate them.

Maybe @franramirez688 can clarify the quoting, as it seems it was part of the necessary solution to the issue.

PhilippFinke1988 commented 2 months ago

I added an example package that reproduces the issue. The package has been created calling conan new foo/0.1 --template=cmake_lib. I added the exelinkflags in method package_info() in conanfile.py and set(CMAKE_VERBOSE_MAKEFILE ON) in the CMakeLists.txt of the test_package.

PhilippFinke1988 commented 2 months ago

I had a closer look into #9936 and think that the issue had nothing to do with spaces in the exelinkflags but rather that those flags have been added to the wrong list. It's true that set(mylib_SHARED_LINK_FLAGS_RELEASE -z now;-z relro) will be converted to a list like -z;now;-z;relro but this shouldn't be an issue as long as the order of the list entries doesn't change. CMake will put spaces between the entries of the list.

I also found out that linker flag -z is ignored by the GNU linker (see https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_3.html). The GNU linker also reports this correctly in the example provided in the ticket. I'm not sure what's the purpose of adding this flag to exelinkflags.

PhilippFinke1988 commented 2 months ago

I think I found a workaround using a different syntax for the -Wl flag:

def package_info(self):
    ....
    self.cpp_info.exelinkflags += ["-Wl,--whole-archive,path/to/libfoo.a,--no-whole-archive"]
    ...

This works for both the deprecated cmake and the CMakeDeps generators.

franramirez688 commented 2 months ago

Hi @PhilippFinke1988

It's great to hear that!

which works in some cases, but most times it reorders the linker flags to something like this

Anyway, I was wondering why this version is working randomly:

def package_info(self):
    ....
    self.cpp_info.exelinkflags += ["-Wl,--whole-archive", "path/to/libfoo.a", "-Wl,--no-whole-archive"]
    ...

Conan is not reordering anything. It's only parsing the Python list and putting all the flags in the same order as they were:

set(mylib_EXE_LINK_FLAGS_RELEASE "-Wl,--whole-archive;path/to/libfoo.a;-Wl,--no-whole-archive")

So, Conan is not causing any issues with this I guess.

PhilippFinke1988 commented 2 months ago

Hey @franramirez688,

I also think that Conan isn't causing the issue but CMake reorders the list under the hood. But I can't figure out how it is reordered.