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

Generate pkg-config contains spurious cflags #2673

Open
NewbiZ opened this issue Sep 18, 2023 · 5 comments
Open

Generate pkg-config contains spurious cflags #2673

NewbiZ opened this issue Sep 18, 2023 · 5 comments
Labels
Cmake Cmake related submissions feature-request A feature should be added or improved. p3 This is a minor priority issue

Comments

@NewbiZ
Copy link

NewbiZ commented Sep 18, 2023

Describe the bug

Context

pkg-config is a Unix tool that allows easy retrieval of compiler and linker flags of dependencies. It relies on .pc files installed in a standard location, and a utility to query flags from your build system.

Issue

The currently generated pkg-config file contains spurious compiler and linker flags that are not meant for users of the library, but rather are a dump of what was used to build the SDK itself.

Expected Behavior

I would expect the following compiler flags for e.g. an S3-only build:

$ pkg-config --cflags aws-cpp-sdk-s3
-I<installation prefix>/include

And the following linker flags:

$ pkg-config --libs aws-cpp-sdk-s3
-L<installation prefix>/lib -laws-cpp-sdk-s3 -laws-cpp-sdk-core

Current Behavior

It seems the current build toolchain just dumps its own build flags in the pkg-config file, instead of the build flags that users of the SDK should use, which means we end up with:

  1. Build-relative paths, which are useless (if not straight up dangerous) for users of the library
  2. Internal compiler flags used to build the AWS C++ SDK itself, which incorrectly leak to the users of the library

More precisly, after an S3-only build, here is the output of pkg-config:

$ pkg-config --cflags aws-cpp-sdk-s3
-fno-exceptions -std=c++11 -fno-exceptions -std=c++11 -Iinclude
  • -fno-exceptions is an implementation choice of the AWS C++ SDK which should not leak to the users of the SDK. Linking exception-aware code with the SDK should be allowed (just as linking with C code from C++ is allowed).
  • -std=c++11 is also an implementation choice of the SDK that should not leak to its users. Using the SDK from a C++ 14 / 17 / 20 / 23 codebase should be supported.
  • -Iinclude is a build-relative path used when building the SDK, it has nothing to do with how users should include the SDK. One can even imagine scenarios where not only this path is useless, but it could also incorrectly include a random directory in users builds.

Reproduction Steps

Any regular build will showcase the incorrect behavior:

$ mkdir build
$ cd build
$ cmake .. -DBUILD_ONLY=s3
$ make
$ make install
$ cat /usr/local/lib/pkgconfig/aws-cpp-sdk-s3.pc
includedir=include
libdir=lib

Name: aws-cpp-sdk-s3
Description: 
Version: 1.11.165
Cflags: -I${includedir} -fno-exceptions -std=c++11
Libs: -L${libdir} -laws-cpp-sdk-s3 
Libs.private: 
Requires:  aws-cpp-sdk-core 
$ cat /usr/local/lib/pkgconfig/aws-cpp-sdk-core.pc 
includedir=include
libdir=lib

Name: aws-cpp-sdk-core
Description: 
Version: 1.11.165
Cflags: -I${includedir} -fno-exceptions -std=c++11
Libs: -L${libdir} -laws-cpp-sdk-core 
Libs.private: -laws-crt-cpp -laws-c-auth -laws-c-cal -laws-c-common -laws-c-compression -laws-c-event-stream -laws-c-http -laws-c-io -laws-c-mqtt -laws-c-s3 -laws-checksums -laws-c-sdkutils -lpthread -lcurl -lcrypto -lssl -lz
Requires: 

Possible Solution

  • Remove SDK-specific build flags (specifically -std=c++11 and -fno-exceptions) here:
    set(PKG_CONFIG_CFLAGS "${_TMP}" CACHE INTERNAL "C++ compiler flags which affect the ABI")
  • Use proper installation directories, not build directories (I guess that would be @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@ or something like that) here:
    includedir=@INCLUDE_DIRECTORY@

Additional Information/Context

No response

AWS CPP SDK version used

Any

Compiler and Version used

Any

Operating System and version

Any linux

@NewbiZ NewbiZ added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 18, 2023
@NewbiZ
Copy link
Author

NewbiZ commented Sep 18, 2023

Also, since the AWS C++ SDK depends internally on libcurl, and defines Libs.private, it seems incoherent to not have libcurl in Requires.private.

@NewbiZ
Copy link
Author

NewbiZ commented Sep 18, 2023

Reading again the CMake macro that generates variables for the pkg-config file, I think the whole part that sets GCC flags should be removed.
All these flags are for builds of the SDK itself and do not make sense for users depending on the SDK, and the pkg-config template already hard-code the only useful flag (-I).

@NewbiZ
Copy link
Author

NewbiZ commented Sep 18, 2023

I would propose the following changes:

  1. Currently, no compiler flag used by the SDK make sense to be propagated to the users. Still, the behavior of filtering this list of flags can be retained. We can then just explicitely remove the flags that should not be propagated:
 function(set_compiler_flags target)
     if(NOT MSVC)
+        # Generate a list of GCC compilation flags
         set_gcc_flags()
         target_compile_options(${target} PRIVATE "${AWS_COMPILER_FLAGS}")
+
+        # Exception support do not affect the ABI
+        list(REMOVE_ITEM AWS_COMPILER_FLAGS "-fno-exceptions")
+        # Language standard do not affect the ABI
+        list(REMOVE_ITEM AWS_COMPILER_FLAGS "-std=c++11")
+        # Optimizations do not affect the ABI
+        list(REMOVE_ITEM AWS_COMPILER_FLAGS "-s")
+
+        # Flatten the list of compilation options to a string for use in pkgconfig
         string(REPLACE ";" " " _TMP "${AWS_COMPILER_FLAGS}")
+
         set(PKG_CONFIG_CFLAGS "${_TMP}" CACHE INTERNAL "C++ compiler flags which affect the ABI")
     endif()
  1. The paths can be trivially fixed by prepending them with the CMAKE_INSTALL_PREFIX. As a side bonus, it is quite common to define prefix in the pkg-config file and have includedir and libdir use it. It then simply becomes:
-includedir=@INCLUDE_DIRECTORY@
-libdir=@LIBRARY_DIRECTORY@
+prefix=@CMAKE_INSTALL_PREFIX@
+includedir=${prefix}/@INCLUDE_DIRECTORY@
+libdir=${prefix}/@LIBRARY_DIRECTORY@

With these two changes, the build and installation of a fresh AWS CPP SDK now generate a proper pkg-config file, as showcased:

$ cat /usr/local/lib/pkgconfig/aws-cpp-sdk-s3.pc 
prefix=/usr/local
includedir=${prefix}/include
libdir=${prefix}/lib

Name: aws-cpp-sdk-s3
Description: 
Version: 1.11.165
Cflags: -I${includedir} 
Libs: -L${libdir} -laws-cpp-sdk-s3 
Libs.private: 
Requires:  aws-cpp-sdk-core

$ cat /usr/local/lib/pkgconfig/aws-cpp-sdk-core.pc 
prefix=/usr/local
includedir=${prefix}/include
libdir=${prefix}/lib

Name: aws-cpp-sdk-core
Description: 
Version: 1.11.165
Cflags: -I${includedir} 
Libs: -L${libdir} -laws-cpp-sdk-core 
Libs.private: -laws-crt-cpp -laws-c-auth -laws-c-cal -laws-c-common -laws-c-compression -laws-c-event-stream -laws-c-http -laws-c-io -laws-c-mqtt -laws-c-s3 -laws-checksums -laws-c-sdkutils -lpthread -lcurl -lcrypto -lssl -lz
Requires: 

$ pkg-config --cflags aws-cpp-sdk-s3
-I/usr/local/include

$ pkg-config --libs aws-cpp-sdk-s3
-L/usr/local/lib -laws-cpp-sdk-s3 -laws-cpp-sdk-core

$ pkg-config --cflags aws-cpp-sdk-core 
-I/usr/local/include

$ pkg-config --libs aws-cpp-sdk-core 
-L/usr/local/lib -laws-cpp-sdk-core

@yasminetalby yasminetalby self-assigned this Sep 18, 2023
@yasminetalby yasminetalby added feature-request A feature should be added or improved. needs-review This issue or pull request needs review from a core team member. Cmake Cmake related submissions and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 18, 2023
@yasminetalby
Copy link
Contributor

Hello @NewbiZ ,

Thank you very much for your detailed submission. We really appreciate your feedback and contribution.

This request falls under the Improvement to build, install, and distribution mechanisms project. We will discuss it as a team as part of this project and decide on implementation and prioritization based on user impact and benefits.

Thank you very much again for your collaboration and for all the details provided.

Sincerely,

Yasmine

@yasminetalby yasminetalby added the p3 This is a minor priority issue label Sep 18, 2023
@jmklix jmklix removed the needs-review This issue or pull request needs review from a core team member. label Nov 17, 2023
@intelfx
Copy link

intelfx commented Mar 21, 2024

@jmklix @yasminetalby I believe this to be a bug rather than a feature request as it breaks dependent projects if they happen to use exceptions. Please review the tags if possible and consider re-prioritizing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cmake Cmake related submissions feature-request A feature should be added or improved. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

4 participants