Skip to content

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Feb 7, 2025

Description of changes:
Hi, I am from the Common Runtime team and we own https://github.com/awslabs/aws-c-common. We recently did some cleanup in CMake, in aws-c-common at awslabs/aws-c-common#1178. This broke this repository because it was relying on aws-c-common to set LIBRARY_DIRECTORY and RUNTIME_DIRECTORY, which aws-c-common no longer does. This PR updates aws-encryption-sdk-c to now use the official CMake variables like aws-c-common.
Changes:

  • Respect CMAKE_INSTALL_LIBDIR, CMAKE_INSTALL_BINDIR, CMAKE_INSTALL_INCLUDEDIR
    • Use these variables directly, instead of defining our own (LIBRARY_DIRECTORY, RUNTIME_DIRECTORY)
    • include(GNUInstallDirs) on all platforms
    • REASONING: This removes a bunch of boilerplate, and makes it obvious we should defer to CMake for install directory names. I assume the original author thought GNUInstallDirs wasn't safe for Windows and Apple, but despite its name, it's fine. Professional CMake says:

      CMake’s GNUInstallDirs module provides a very convenient way to use a standard directory layout. ... it also provides various other standard locations that conform to both GNU coding standards and the FHS. ... the layout can even be used for Windows deployments

We can improve the CMake further like we did in aws-c-common, which will also fix #811, but I have decided to keep this PR simple, so that it is easier to review and merge.

  • macOS-11 is no longer available, use the minimum macOS-13 github runner.
  • Use macos-latest-large which is the Intel based runner instead of macos-latest which is now arm based.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

@waahm7 waahm7 requested a review from a team as a code owner February 7, 2025 19:23
@waahm7 waahm7 had a problem deploying to continuous-integration February 7, 2025 20:23 — with GitHub Actions Failure
@waahm7 waahm7 had a problem deploying to continuous-integration February 7, 2025 20:24 — with GitHub Actions Error
Copy link

@graebm graebm left a comment

Choose a reason for hiding this comment

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

LGTM (I had made the changes to aws-c-common)

@waahm7 waahm7 had a problem deploying to continuous-integration February 7, 2025 21:43 — with GitHub Actions Failure
@waahm7 waahm7 had a problem deploying to continuous-integration February 7, 2025 21:43 — with GitHub Actions Error
@waahm7 waahm7 had a problem deploying to continuous-integration February 10, 2025 18:25 — with GitHub Actions Failure
@waahm7 waahm7 had a problem deploying to continuous-integration February 10, 2025 18:26 — with GitHub Actions Error
@ajewellamz ajewellamz changed the title CMake Fixes to Make it Compatible with Latest Aws-c-common fix: CMake Fixes to Make it Compatible with Latest Aws-c-common Feb 10, 2025
@waahm7 waahm7 had a problem deploying to continuous-integration February 10, 2025 18:51 — with GitHub Actions Failure
@waahm7 waahm7 had a problem deploying to continuous-integration February 10, 2025 18:52 — with GitHub Actions Error
@waahm7 waahm7 temporarily deployed to continuous-integration February 10, 2025 19:06 — with GitHub Actions Inactive
@waahm7 waahm7 temporarily deployed to continuous-integration February 10, 2025 19:06 — with GitHub Actions Inactive
Copy link
Contributor

@ajewellamz ajewellamz left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ajewellamz ajewellamz merged commit 1af76aa into aws:master Feb 10, 2025
7 checks passed
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.

3 participants