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

Add cxx support #294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add cxx support #294

wants to merge 1 commit into from

Conversation

subdiox
Copy link

@subdiox subdiox commented Oct 18, 2024

Issue #, if available: #166

Description of changes: This PR fixes the build error which occurs when it's built using Swift Package Manager (SPM) with C++ interoperability mode enabled.
Following changes were made:

  • AWS_NO_STATIC_IMPL is defined when it's built with C++ complier.
  • Extracted enum to the root
  • Cast type explicitly if needed
  • Avoid using template keyword as a variable name

Dev Test:
I have confirmed that this package could be built successfully with C++ interoperability enabled.

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

@waahm7
Copy link
Contributor

waahm7 commented Oct 18, 2024

Thank you for creating the PR. These changes will need to happen in the submodules first. I have created a PR at awslabs/aws-c-common#1160 that solves this problem a bit differently. Once that is approved and merged, I will create PRs for the remaining submodules.

@subdiox
Copy link
Author

subdiox commented Oct 19, 2024

@waahm7 Thank you so much for quick response. Can I create PRs for other submodules to be followed by myself?

@waahm7
Copy link
Contributor

waahm7 commented Oct 21, 2024

@subdiox Sure, we will also need a CI job that tests crt-swift with cxx-interop before the final merge.

@subdiox
Copy link
Author

subdiox commented Nov 5, 2024

@waahm7 I put up three PRs regarding the submodules. Could you review them if possible?

@waahm7
Copy link
Contributor

waahm7 commented Nov 11, 2024

@subdiox Thanks, I have merged all three PRs. We should use the latest submodules in this PR and a test for the cxx interop.

@subdiox subdiox reopened this Nov 11, 2024
@subdiox
Copy link
Author

subdiox commented Nov 11, 2024

@waahm7 I have rebased the submodules to update the related ones to the HEAD main. Could you review it?

@waahm7
Copy link
Contributor

waahm7 commented Nov 11, 2024

@subdiox We will also need to update CI to test the cxx interop before we can merge this.

@subdiox
Copy link
Author

subdiox commented Nov 11, 2024

@waahm7 Should I add -cxx-interoperability-mode=default option for each build step in the GitHub Actions? Or do you have a plan to update the test?

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.

2 participants