Skip to content

build dev 1.10 pr7 #1933

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2022
Merged

build dev 1.10 pr7 #1933

merged 1 commit into from
May 20, 2022

Conversation

sdavtaker
Copy link
Contributor

@sdavtaker sdavtaker commented May 20, 2022 •
edited
Loading

Issue #, if available: 1888

Description of changes:

  • Adding support for inferring version validating build_types

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
    No, but there were already.
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

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

@sdavtaker sdavtaker changed the base branch from main to build-dev-1.10 May 20, 2022 19:35
@sdavtaker sdavtaker force-pushed the build-dev-1.10-pr7 branch from 76776e4 to b020bb5 Compare May 20, 2022 19:38
@sdavtaker sdavtaker requested a review from sbiscigl May 20, 2022 19:38
sdavtaker
sdavtaker commented May 20, 2022
View reviewed changes
Copy link
Contributor Author

@sdavtaker sdavtaker left a comment

Choose a reason for hiding this comment

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

Commented on the changes.

CMakeLists.txt
@@ -17,40 +17,57 @@ if (LEGACY_BUILD)
project("aws-cpp-sdk-all" VERSION "${PROJECT_VERSION}" LANGUAGES CXX)
include(legacy_main)
else ()
message(STATUS "Building with 1.10 CMake scripts.")
message(STATUS "Building with new CMake scripts.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking in 1.11 release it may not make sense to keep updating this value.

CMakeLists.txt
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

message(STATUS "Building ${PROJECT_NAME} ${CMAKE_PROJECT_VERSION}")

# Setting version header for library
configure_file(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Producing the VersionConfig header for the build.
Different than in LEGACY_BUILD this header is not overriding SRC files, it is produced into the BUILD.
The SRC file version from 1.9 is marked as deprecated for future removal.

CMakeLists.txt
Comment on lines +53 to +64
get_property(is_multi_conf_build GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
if (NOT is_multi_conf_build)
set(allowed_build_types Debug Release RelWithDebInfo MinSizeRel)
# cmake-gui helper
set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "${allowed_build_types}")
if (NOT CMAKE_BUILD_TYPE)
message(STATUS "Setting build type to 'RelWithDebInfo' as none was specified.")
set(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "Choose the type of build." FORCE)
elseif (NOT CMAKE_BUILD_TYPE IN_LIST allowed_build_types)
message(FATAL_ERROR "Unknown build type: ${CMAKE_BUILD_TYPE}")
endif ()
endif ()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validating that the right build type is requested and setting the default to RelWithDeb if omitted.

CMakeLists.txt
message(FATAL_ERROR "Unknown build type: ${CMAKE_BUILD_TYPE}")
endif ()
endif ()
option(BUILD_TESTS "If enabled, the SDK will include tests in the build" OFF)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, no tests are build, creating option to enable them in the build.

VersionConfig.h.in
Comment on lines +6 to +10
#define AWS_CPP_SDK_VERSION_STRING "@aws-cpp-sdk_VERSION_STRING@"
#define AWS_CPP_SDK_VERSION_MAJOR @aws-cpp-sdk_VERSION_MAJOR@
#define AWS_CPP_SDK_VERSION_MINOR @aws-cpp-sdk_VERSION_MINOR@
#define AWS_CPP_SDK_VERSION_PATCH @aws-cpp-sdk_VERSION_PATCH@
#define AWS_CPP_SDK_GIT_HASH "@aws-cpp-sdk_GIT_HASH@"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added CPP tot he prefix to avoid collisions with defines in other SDKs that may be build together.

cmake/project_version.cmake
Comment on lines +4 to +19
if (GIT_FOUND)
execute_process(
COMMAND ${GIT_EXECUTABLE} --git-dir=${CMAKE_CURRENT_SOURCE_DIR}/.git describe --abbrev=0 --tags
OUTPUT_VARIABLE VERSION_STRING
OUTPUT_STRIP_TRAILING_WHITESPACE
)
execute_process(
COMMAND ${GIT_EXECUTABLE} rev-parse HEAD
RESULT_VARIABLE git_result
OUTPUT_VARIABLE GIT_HASH
OUTPUT_STRIP_TRAILING_WHITESPACE
)
if (NOT git_result)
set(${resultVarGitHash} ${GIT_HASH} PARENT_SCOPE)
message(STATUS "Building git hash: ${GIT_HASH}")
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In dev environments, inside git repository, the version will be inferred from tags and git hash saved to the build.

cmake/project_version.cmake
Comment on lines +22 to +24
if (NOT VERSION_STRING) # Non DEV build relays on VERSION file updated by release CI
file(READ "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" VERSION_STRING)
endif ()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In non dev builds, those with no git, the version will be read from a VERSION file in the root of the SDK sourcetree. This file is expected to be updated by CI at the release process. The placement in the root is to have easy access to it for human readers.

src/aws-cpp-sdk-core/include/aws/core/Version.h
@@ -15,5 +15,6 @@ namespace Version
AWS_CORE_API unsigned GetVersionMinor();
AWS_CORE_API unsigned GetVersionPatch();
AWS_CORE_API const char* GetCompilerVersionString();
AWS_CORE_API const char* GetGitHash();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For development builds, on releases this is empty.

src/aws-cpp-sdk-core/include/aws/core/VersionConfig.h
Comment on lines +12 to +17
// Forward compatible definitions
#define AWS_CPP_SDK_VERSION_STRING AWS_SDK_VERSION_STRING
#define AWS_CPP_SDK_VERSION_MAJOR AWS_SDK_VERSION_MAJOR
#define AWS_CPP_SDK_VERSION_MINOR AWS_SDK_VERSION_MINOR
#define AWS_CPP_SDK_VERSION_PATCH AWS_SDK_VERSION_PATCH
#define AWS_CPP_SDK_GIT_HASH "NoHashAvailableInLegacyBuild"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for forward compatibility.

src/aws-cpp-sdk-core/include/aws/core/VersionConfig.h.in
Comment on lines +12 to +17
#define AWS_CPP_SDK_VERSION_STRING AWS_SDK_VERSION_STRING
#define AWS_CPP_SDK_VERSION_MAJOR AWS_SDK_VERSION_MAJOR
#define AWS_CPP_SDK_VERSION_MINOR AWS_SDK_VERSION_MINOR
#define AWS_CPP_SDK_VERSION_PATCH AWS_SDK_VERSION_PATCH
#define AWS_CPP_SDK_GIT_HASH "NoHashAvailableInLegacyBuild"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for forward compatibilty

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know CPP SDK sounds better, but we have AWS SDK CPP in most of the places.
nit 2: maybe GIT_SHA1 or _REVISION or GIT_TAG?

@sdavtaker sdavtaker marked this pull request as ready for review May 20, 2022 19:51
@sdavtaker sdavtaker requested a review from SergeyRyabinin May 20, 2022 19:52
@sdavtaker sdavtaker merged commit b06b085 into build-dev-1.10 May 20, 2022
@sdavtaker sdavtaker deleted the build-dev-1.10-pr7 branch May 20, 2022 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbiscigl sbiscigl sbiscigl approved these changes

@SergeyRyabinin SergeyRyabinin SergeyRyabinin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sdavtaker @sbiscigl @SergeyRyabinin