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

chore: remove module.modulemap #3961

Merged
merged 1 commit into from
Apr 27, 2023
Merged

chore: remove module.modulemap #3961

merged 1 commit into from
Apr 27, 2023

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Apr 26, 2023

awslabs/aws-crt-swift#163

Description of changes:

What is module.modulemap
module.modulemap allows C code to be consumed as a clang module. Since Swift consumes s2n-tls as a clang module this file is/was necessary (this file was added by the SDK team to support Swift build). However, more recent version of Swift Packet Manager now supports auto-generating the module.modulemap so this file is not necessary.

Why remove module.modulemap
In some environments (case-insensitive file systems) we are seeing issues. Note that this is the SDK build team (same team that originally added this file) and they are asking to remove the module.modulemap since its can be auto generated from the Swift Packet Manager.

Impact
module.modulemap enables s2n-tls code to be consumes as a clang module. This therefore impacts two types of build systems: Swift and Clang.

  • Swift: the impact here should be low since customers would need to be running an old version of Swift Packet Manager and typical Apple development is quite aggressive about updating development version. The risk here is non-zero but small; additionally our only known customer (SDK team) wants this change.
  • Clang: a clang build system can always work around this issue by providing their own custom module.modulemap and passing custom flags to the clang compiler. The impact here is low and customers have a work around. (Note that passing custom flags to the Swift PM is not possible, hence why we needed to include module.modulemap in our repo previously.)

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

@github-actions github-actions bot added the s2n-core team label Apr 26, 2023
@toidiu toidiu changed the title chore: allow customers to autogenerate module.modulemap themselves chore: remove module.modulemap Apr 26, 2023
@dougch dougch self-requested a review April 26, 2023 17:03
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

What effect will this have on existing Swift customers building the library? Will it break existing builds?

Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

A doc issue or quick note somewhere about swift usage would be helpful.

@toidiu
Copy link
Contributor Author

toidiu commented Apr 26, 2023

What effect will this have on existing Swift customers building the library? Will it break existing builds?

As I understand the swift build step auto-generates this file so it should not affect customers.

I have updated the description with a more detailed description of actions and summary

@toidiu
Copy link
Contributor Author

toidiu commented Apr 26, 2023

A doc issue or quick note somewhere about swift usage would be helpful.

We dont officially support swift bindings and I am not aware of all the steps and files needed for swift bindings so I am wary of adding guidance which might be half baked.

@toidiu toidiu requested a review from lrstewart April 27, 2023 00:07
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

What effect will this have on existing Swift customers building the library? Will it break existing builds?

The description was updated to answer this. This change could break customer builds, but the expected impact is still low and affected customers have workarounds.

@toidiu toidiu enabled auto-merge (squash) April 27, 2023 18:23
@toidiu toidiu merged commit a7e56c8 into aws:main Apr 27, 2023
@toidiu toidiu deleted the ak-modulemap branch April 27, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants