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 176 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.

@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
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@neonichu neonichu removed their assignment Sep 5, 2023
@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor

@swift-ci test linux

1 similar comment
@MaxDesiatov
Copy link
Contributor

@swift-ci test linux

@code-per-day
Copy link

What's the status on this? Will the pitch return?

@ncooke3
Copy link
Contributor Author

ncooke3 commented Dec 6, 2023

Hi @code-per-day, thanks for commenting.

What's the status on this?

This proposal is preparing for a second round review. Following the first review, there was some feedback that needed to get addressed before running it through a second review. While I've addressed most of it, there are some remaining todos involving the C++ toolchain and testing Windows support. I hope to have these bits finalized soon.

Will the pitch return?

Yes, I plan to run this through a second review when the above items are addressed.

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

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.

None yet

7 participants