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.

Sign up for GitHub

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

Jump to bottom

SwiftProtobufPlugin: Add support for ProtoPathModuleMappings swift opt #1450

Open
ripplexyz opened this issue Aug 26, 2023 · 4 comments
Open

SwiftProtobufPlugin: Add support for ProtoPathModuleMappings swift opt #1450

ripplexyz opened this issue Aug 26, 2023 · 4 comments

Comments

@ripplexyz
Copy link

Can we add support for https://github.com/apple/swift-protobuf/blob/main/Documentation/PLUGIN.md#generation-option-protopathmodulemappings---swift-module-names-for-proto-paths in SwiftProtobufPlugin?

This option allows us to distribute proto files in multiple swift modules.

For example:
Core

  • Empty.proto
  • Media.proto

Auth

  • Session.proto
  • User.proto

And Auth SPM package depends on Core SPM package.
@FranzBusch
Copy link
Member

Are those modules in the same package or different packages?

When I implemented the plugins I tried to add cross module package dependency support but we are currently not having the right information in the plugin to make this work. I also think we do not want to use ProtoPathModule mappings for this but rather just rely on the Swift module names and infer it from there.

@thomasvl
Copy link
Collaborator

If SwiftPM doesn't expose what's needed for this automatically, should we do something in the mean time?

@thomasvl
Copy link
Collaborator

@FranzBusch @tbkka Thoughts on adding something for folks in the mean time? If/when SwiftPM gets the support to not need this, we can always do a major version bump and drop it. Without something, it seems like you can't have protos in one module depend on protos provided from another module.

@FranzBusch
Copy link
Member

I don't think we should add support for the ProtoPathModuleMapping in the SPM plugin. We ought to be able to model this differently with SPM especially in the face of Xcode plugins that just have a totally different semantic of targets than SPM targets. We always need to make sure that the files are part of a target and we cannot just pass arbitrary paths to the generated build commands. However, the current ProtoPathModuleMapping just works on arbitrary paths.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Jan 5, 2024
@thomasvl
Add a CompileTest for "multi module" generation.
a76c689 
- Add basic infrastructure to have Compile Tests.

- Add a specific MultiModule CompileTest.
  - This ensure that with `import public` is properly handle and the generated
    code compiles even when a needed type is coming from a public import.
  - So the any relevant code changes are more visible, check the the generated
    code so the diffs for an changes related to the test will be easily
    reviewed.
  - The SPM plugin doesn't support multi module generation
    (apple#1450), which is also why the
    code needs to be checked in, otherwise, it might simplify the tests.

Note: The whole concept of CompileTests doesn't have to be a set up as distinct
Swift Packages, but if we put the required targets in the main Package.swift,
all projects using swift-protobuf would end up having the "overhead" of those
test only targets, so I've errored on the side of a distinct package to make
sure there is no impact of these tests on the folks using swift-protobuf.
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Jan 8, 2024
@thomasvl
Add a CompileTest for "multi module" generation.
5c215b2 
- Add basic infrastructure to have Compile Tests.

- Add a specific MultiModule CompileTest.
  - This ensure that with `import public` is properly handle and the generated
    code compiles even when a needed type is coming from a public import.
  - So the any relevant code changes are more visible, check the the generated
    code so the diffs for an changes related to the test will be easily
    reviewed.
  - The SPM plugin doesn't support multi module generation
    (apple#1450), which is also why the
    code needs to be checked in, otherwise, it might simplify the tests.

Note: The whole concept of CompileTests doesn't have to be a set up as distinct
Swift Packages, but if we put the required targets in the main Package.swift,
all projects using swift-protobuf would end up having the "overhead" of those
test only targets, so I've errored on the side of a distinct package to make
sure there is no impact of these tests on the folks using swift-protobuf.
@thomasvl thomasvl mentioned this issue Jan 10, 2024
Merged
thomasvl added a commit that referenced this issue Jan 11, 2024
@thomasvl
Add a CompileTest for "multi module" generation.
2a383b6 
- Add basic infrastructure to have Compile Tests.

- Add a specific MultiModule CompileTest.
  - This ensure that with `import public` is properly handle and the generated
    code compiles even when a needed type is coming from a public import.
  - So the any relevant code changes are more visible, check the the generated
    code so the diffs for an changes related to the test will be easily
    reviewed.
  - The SPM plugin doesn't support multi module generation
    (#1450), which is also why the
    code needs to be checked in, otherwise, it might simplify the tests.

Note: The whole concept of CompileTests doesn't have to be a set up as distinct
Swift Packages, but if we put the required targets in the main Package.swift,
all projects using swift-protobuf would end up having the "overhead" of those
test only targets, so I've errored on the side of a distinct package to make
sure there is no impact of these tests on the folks using swift-protobuf.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@FranzBusch @thomasvl @ripplexyz