Remove -Wl,-rpath from AutotoolsDeps/GnuDeps/PkgConfigDeps
#10192
Conversation
SpaceIm
force-pushed
the
fix/pkgconfig-no-rpath
branch
from
5674345
to
213721d
Compare
SpaceIm
changed the title
pkg_config and PkgConfigDepspkg_config and PkgConfigDeps
There was a problem hiding this comment.
If you could elaborate a bit further why these flags have to be removed from pkg_config only, that would be very helpful. These flags are also being applied to autotools helpers (both legacy and new), and to general compiler_flags, that could be used by other user integrations.
| @@ -16,7 +16,7 @@ | |||
| Name: my-project | |||
| Description: Some brief but informative description | |||
| Version: 1.2.3 | |||
| Libs: -L${libdir} -lmy-project-1 -linkerflag -Wl,-rpath=${libdir} | |||
There was a problem hiding this comment.
The thing is that if this is here, it was probably requested by some users, and it might be breaking for them. Maybe it would be better to not touch the pkg_config and only change PkgConfigDeps
Also taking into account that other generators, like autotools (both the legacy and the new conan.tools.gnu ones) also apparently add these flags.
There was a problem hiding this comment.
It's explained in the linked issue, those flags are never added to official pkgconfig files nor CMake config files. If it breaks some people, they are doing something wrong.
Adding these flags has side effects in executables & shared libs built with dependencies discovered through these pkgconfig files.
There was a problem hiding this comment.
I've found this PR adding rpath #2797 among other modifications (seems to be the first PR after first introduction of pkg_config generator), there is a request to add it in this comment #2756 (comment) without much rational.
/cc @coffee-lord and @danimtb if you can remember why -Wl,-rpath has been added to .pc files.
There was a problem hiding this comment.
I guess that it should also be removed from AutoToolsBuildEnvironment helper and AutotoolsDeps generator, but I didn't want to change too many conan tools in the same PR.
It's worth noting that it's optional in AutoToolsBuildEnvironment, defaulted to False, and CCI recipes never enable rpath in this helper.
There was a problem hiding this comment.
It's explained in the linked issue, those flags are never added to official pkgconfig files nor CMake config files. If it breaks some people, they are doing something wrong.
It is not enough to say that they are doing something wrong. Conan is doing it, and it has been doing it for years. If they rely on it (and I can guarantee some people will) and Conan removes it, it will be breaking behavior, and we cannot do that. Unless it is declared a clear bug (which I doubt can be done), then it is just not the best default behavior, but that cannot be used to break (in the same way we haven't been able to change libcxx=libstdc++11 default).
It's worth noting that it's optional in AutoToolsBuildEnvironment, defaulted to False, and CCI recipes never enable rpath in this helper.
Yes, same, this probably cannot be removed without being breaking, even if not used in CCI, it can be used by users own recipes.
What is clear, is that if this is to be removed, we should make sure it is being removed from GnuDepsFlags too and not only PkgConfigDeps
There was a problem hiding this comment.
It's a bug because pkg_config and PkgConfigDeps aim to emulate pkgconfig files, and no one adds -Wl-rpath in pkgconfig files generated for a lib (I refer to official pkgconf files generated by build systems), because it doesn't make sense to make this decision for consumers.
So it's different than AutoToolsBuildEnvironment which is pure conan construction (and I recommend to not add -Wl,-rpath to GnuDepsFlags or AutotoolsDeps, or if you want to keep this feature, add an option off by default).
As a consequence, when you consume these pkgconfig files in your build system, it will hardcodes path of dependencies in rpath of generated runtime files. It should not be the default behavior, specially for conan whose purpose is to distribute relocatable files.
Now I understand that it could break some people, not sure to understand why someone would rely on this flags coming from pkgconfig files, instead of expecting what a normal pkgconfig file should provide. So how to procede? Add an option in PkgConfigDeps? What about pkg_config?
There was a problem hiding this comment.
That decision was made for consumers 3 years ago. Maybe it was not the right decision, but it was done, as a feature.
I am very happy to remove it from PkgConfigDeps, and we can try to remove it from pkg_config but if users complain that we are breaking something that was working in the past, then, they will not care that the official pkgconfig files by build systems do differently, they have been trusting Conan to do this for them for years, and now we will break their builds.
You can say exactly the same for libcxx=libstdc++ by default, that it is a bug, because it should be ``libstdc++11++ which is the compiler default. But the truth is that Conan was working well for them, building and running correctly, and if we change that, their builds will completely break. The thing is that it is not intrinsically a bug, but a bad default, which is not the same.
I 100% understand your point, but honestly, I am very tired of users complaining because we "broke" something that was working in the past, I'd prefer to avoid that possibility as much as possible, because it is very time and energy consuming for us (that typically implies having to do a patch release).
There was a problem hiding this comment.
Ok, so would it be ok to remove it in new generators of conan v2 (not stable yet), or add an option off by default?
There was a problem hiding this comment.
It can be removed from new generators without needing to add an option. Lets do the right thing this time, and if someone request this feature for the new generators, we will make sure to understand the use case and see if it really makes sense, and it is the correct place (in any case, it would be more a "toolchain" thing IMO)
SpaceIm
changed the title
pkg_config and PkgConfigDepsAutotoolsDeps/GnuDeps/PkgConfigDeps/CompilerArgs
| @@ -2,7 +2,7 @@ | |||
| build_type_flags, format_defines, | |||
| format_include_paths, format_libraries, | |||
| format_library_paths, libcxx_define, libcxx_flag, | |||
| rpath_flags, sysroot_flag, | |||
| sysroot_flag, | |||
There was a problem hiding this comment.
compiler_args is a legacy generator, not a new one, better leave it as-is
SpaceIm
changed the title
AutotoolsDeps/GnuDeps/PkgConfigDeps/CompilerArgsAutotoolsDeps/GnuDeps/PkgConfigDeps
|
Many thanks for contributing this! |
Changelog: Fix: remove rpath from .pc files generated by
pkg_config&PkgConfigDepsgenerators.Docs: conan-io/docs#2339
developbranch, documenting this one.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.
closes #7878