Skip to content

cmake: don't set PACKAGE_VERSION in Config.cmake #1080

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

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

flixr
Copy link
Contributor

@flixr flixr commented Mar 1, 2019

don't include the ConfigVersion.cmake in Config.cmake
as this sets e.g. PACKAGE_VERSION in other projects find_package-ing
AWSSDK.

Use AWSDK_VERSION for printing the found version instead it is supposed
to be.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

don't include the ConfigVersion.cmake in Config.cmake
as this sets e.g. PACKAGE_VERSION in other projects find_package-ing
AWSSDK.

Use AWSDK_VERSION for printing the found version instead it is supposed
to be.
@flixr
Copy link
Contributor Author

flixr commented Apr 7, 2019

friendly ping...

@flixr
Copy link
Contributor Author

flixr commented Apr 28, 2020

ping...

@sdavtaker
Copy link
Contributor

Hi @flixr
I was looking into the PR, and it seems from CMake doc: https://cmake.org/cmake/help/latest/command/find_package.html#version-selection
The PACKAGE_VERSION is not alive after find_packaging.
Can you provide a short code-sample showing the bug?

@flixr
Copy link
Contributor Author

flixr commented Aug 19, 2021

The PACKAGE_VERSION is not alive after find_packaging.

Unless you explicitly include the version file in the config file... which you should you not do and is not needed.

A minimal sample showing this should look something like this:

cmake_minimum_required (VERSION 3.1)

project(my_project 1.0)

find_package(AWSSDK REQUIRED COMPONENTS transfer)

message(STATUS "my_project version: ${PROJECT_VERSION}")

Now PROJECT_VERSION is not the version of my_project, but of the AWSSDK

@jmklix
Copy link
Member

jmklix commented Apr 22, 2022

#1888

@specious
Copy link
Contributor

To be precise, project(<project_name> VERSION <version>) sets PROJECT_VERSION.

Meanwhile, find_package(AWSSDK) sets an unrelated PACKAGE_VERSION, which it sets globally.

It does make sense to rename it to AWSSDK_VERSION, but it does not conflict with PROJECT_VERSION.

CMakeLists.txt:

cmake_minimum_required(VERSION 3.5)

project(my_project VERSION 1.1)

message(STATUS "PACKAGE_VERSION: ${PACKAGE_VERSION}")
message(STATUS "PROJECT_VERSION: ${PROJECT_VERSION}")

find_package(AWSSDK REQUIRED s3)

message(STATUS "PACKAGE_VERSION: ${PACKAGE_VERSION}")
message(STATUS "PROJECT_VERSION: ${PROJECT_VERSION}")

find_package(PkgConfig)

message(STATUS "PACKAGE_VERSION: ${PACKAGE_VERSION}")
message(STATUS "PROJECT_VERSION: ${PROJECT_VERSION}")

Output:

$ cmake . -DCMAKE_PREFIX_PATH=/where/aws-sdk-cpp/is/installed

-- The C compiler identification is GNU 11.2.1
-- The CXX compiler identification is GNU 11.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- PACKAGE_VERSION:
-- PROJECT_VERSION: 1.1
-- Found AWS SDK for C++, Version: 1.9.234, Install Root:/opt/aws, Platform Prefix:, Platform Dependent Libraries: pthread;crypto;ssl;z;curl
-- Components specified for AWSSDK: s3, application will be depending on libs: aws-cpp-sdk-s3;aws-cpp-sdk-core;aws-crt-cpp;aws-c-auth;aws-c-cal;aws-c-common;aws-c-compression;aws-c-event-stream;aws-c-http;aws-c-io;aws-c-mqtt;aws-c-s3;aws-checksums;pthread;crypto;ssl;z;curl
-- Try finding aws-cpp-sdk-core
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Found LibCrypto: /usr/lib/libcrypto.so
-- LibCrypto Include Dir: /usr/include
-- LibCrypto Shared Lib:  /usr/lib/libcrypto.so
-- LibCrypto Static Lib:  /usr/lib/libcrypto.so

... [ more output ]

-- LibCrypto Shared Lib:  /usr/lib/libcrypto.so
-- LibCrypto Static Lib:  /usr/lib/libcrypto.so
-- Found aws-cpp-sdk-core
-- Try finding aws-cpp-sdk-s3
-- Found aws-cpp-sdk-s3
-- PACKAGE_VERSION: 1.9.234
-- PROJECT_VERSION: 1.1
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.8.0")
-- PACKAGE_VERSION: 1.9.234
-- PROJECT_VERSION: 1.1
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/work

@specious
Copy link
Contributor

specious commented Aug 19, 2022

Actually, AWSSDK_VERSION is also being set, as this will show:

cmake_minimum_required(VERSION 3.5)

project(my_project)

find_package(AWSSDK)
message(STATUS "AWSSDK_VERSION: ${AWSSDK_VERSION}") 

@flixr
Copy link
Contributor Author

flixr commented Aug 24, 2022

yes, and to not set PACKAGE_VERSION globally (which was only used to print the AWSSDK_VERSION), the config version file should not be included in the config file.
That is fixed by this PR

@specious
Copy link
Contributor

include(${CMAKE_CURRENT_LIST_DIR}/AWSSDKConfigVersion.cmake)

I can't seem to figure out where this .cmake file actually comes from.

@flixr
Copy link
Contributor Author

flixr commented Aug 24, 2022

IIRC this is automatically created by cmake and this should not be include in the XConfig.cmake file

@flixr
Copy link
Contributor Author

flixr commented Jan 25, 2023

@specious can this be merged?
The the version file should not be included and it's not needed... the only var that is used from there is PROJECT_VERSION and that is only used to print the message.

With the current CMake config file it messes up any project where we find_packge awssdk!

@specious
Copy link
Contributor

I'm not a maintainer of this project and I'm not sure what the correct decision would be.

@SergeyRyabinin
Copy link
Contributor

@sdavtaker , could you pleas take a look at this PR?

@SergeyRyabinin SergeyRyabinin merged commit 682fe3f into aws:master Jun 5, 2023
@flixr flixr deleted the fix_cmake_config branch June 9, 2023 16:33
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.

5 participants