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 support for targets with mixed language sources #5919

Open
wants to merge 193 commits into
base: main
Choose a base branch
from

Conversation

ncooke3
Copy link
Contributor

@ncooke3 ncooke3 commented Nov 21, 2022

Add support for targets with mixed language sources.

Motivation:

At the time of this PR, the package manager does not support targets with mixed language sources. This feature was motivated by the need for packages to contain mixed language sources for legacy and technical reasons.

Modifications:

Detailed design is explained in swiftlang/swift-evolution#1895

The diff is large but most of it comes from the suite of integration tests. I added many fixture tests to test many cases that arise when making a mixed target.

High Level Overview

  1. Added a MixedTarget type to model targets with mixed language sources
    • The current approach is to effectively wrap a SwiftTarget and ClangTarget.
  2. Added a MixedTargetBuildDescription type to model how MixedTarget's should build.
    • The current approach is to effectively wrap a SwiftTargetBuildDescription and ClangTargetBuildDescription.
  3. Expanded upon the existing VFSOverlay type to support VFS overlay directories.
    • Used by MixedTargetBuildDescription when creating the VFS overlays needed to compile a mixed language target.
  4. Expanded upon the existing ModuleMapGenerator implementation to generate module maps for mixed language targets. This involves adding a submodule to the module map to expose the generated Swift interop header.
  5. Modify the PackageBuilder to created a mixed language target instead of throwing an error when mixed language sources are detected (when sufficient Swift tools version is detected).
  6. Add unit tests to test planning a mixed target, mixed target module map generation, and building a mixed target.
  7. Add extensive suite of integration (in FunctionalTests target) tests to test mixed target use cases.

Result:

Targets with mixed language sources can be built, tested, and depended on by other packages.


Development Workflow

I have added info to this Google Doc for instructions on how to use this implementation to build and test mixed language targets.

@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@neonichu
Copy link
Contributor

Thanks a lot for this, @ncooke3!

The changes look good to me in general, I'll do are more in-depth review once we're closer to landing this. It would be good to unify mixed targets and the individual language targets at some point, but it seems reasonable to start this way.

@ncooke3 ncooke3 force-pushed the nc/support-mixed-targets branch from 60e115a to 76c5303 Compare December 1, 2022 19:04
@neonichu
Copy link
Contributor

neonichu commented Dec 2, 2022

One bit I hadn't thought about until now is that we'd need to make the support conditional on the tools-version since we would not want authors accidentally creating packages that aren't actually backwards compatible. For existing tools-versions, we would probably want to preserve the error message about mixed sources not being supported.

@ncooke3 ncooke3 force-pushed the nc/support-mixed-targets branch 4 times, most recently from 6083a00 to 28a527c Compare December 15, 2022 20:08
@ncooke3
Copy link
Contributor Author

ncooke3 commented Dec 19, 2022

In 9456663, I added a suite of integration-style tests to document and test the behavior of mixed targets. Along the way, I tweaked the implementation, and added TODOs to further refine the implementation. Currently, not all of the added tests are passing– there are some gaps that I will be filling in over the next few days.

I'm happy to break the PR up once it is ready for review.

Copy link

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Looks great! Just some miscellaneous nits ....

@ncooke3 ncooke3 marked this pull request as ready for review December 28, 2022 23:17
@ncooke3 ncooke3 force-pushed the nc/support-mixed-targets branch 2 times, most recently from d0d6484 to 41770f7 Compare January 12, 2023 22:52
@ncooke3 ncooke3 changed the base branch from main to 57MakeREPLWorkAgain January 14, 2023 21:36
@ncooke3 ncooke3 changed the base branch from 57MakeREPLWorkAgain to main January 14, 2023 21:36
@ncooke3 ncooke3 force-pushed the nc/support-mixed-targets branch 2 times, most recently from 2e22eb1 to 80a9c8b Compare January 14, 2023 22:09
@MaxDesiatov MaxDesiatov requested review from MaxDesiatov and removed request for neonichu, tomerd, elsh and abertelrud August 8, 2024 19:51
@MaxDesiatov
Copy link
Contributor

@swift-ci test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants