Always use PathToUnderscores naming for SwiftPM plugin outputs#2090
Always use PathToUnderscores naming for SwiftPM plugin outputs#2090adityasingh2400 wants to merge 4 commits into
Conversation
| /// - fileNaming: The file naming strategy, or `nil` for the default | ||
| /// (`FullPath`). | ||
| /// - Returns: The relative path of the generated `.pb.swift` file. | ||
| static func outputFileName( |
There was a problem hiding this comment.
@FranzBusch @glbrntt - Should we consider moving the options into the plugin target or making a PluginOptions target that just have these enums and the backing code for them so it can be shared between the SwiftPM plugin and protoc generator (and then maybe also used by the grpc, etc.) to avoid having duplicate copies of the names/functionality?
There was a problem hiding this comment.
Sadly it isn't possible to share code between a plugin and another target directly. Swift PM doesn't support dependencies in plugins. The only thing we could do is copy or symlink
There was a problem hiding this comment.
Wait, can plugins not depend on other libraries to share code? Guess that means we do have to duplicate stuff. We could share it between the protoc generators, but looks like SwiftPM plugins might have to be self contained? Ugh.
There was a problem hiding this comment.
Ha, looks like we just both replied at the same time.
Symlink seems likely to break when making edits based on what someone's editor does for saves.
Copy has the risk of drifting, we could have the Makefile check that they stay in sync, but not great. Think either is worth it?
There was a problem hiding this comment.
And even if we share it via the symlink/copy here, that wouldn't help something like grpc (the SwiftPM plugin). As there the copy/symlink becomes even more of a maintenance issue.
There was a problem hiding this comment.
I don't think there's actually any value in giving users a choice for file naming when using the build plugin as the outputs go into the build dir.
We should just always use path-to-underscores (this is what gRPC does). This also means the transformation is straightforward which removes the maintenance problem.
There was a problem hiding this comment.
Agreed, this is the right call. I reworked the PR to always use PathToUnderscores.
The protoc invocation is now pinned to FileNaming=PathToUnderscores, and the declared output names are derived with the same transformation (drop .proto, replace the directory separators with underscores). Because both the generator invocation and the declared outputs use one strategy, they can never diverge. The mirrored copy of FileGenerator.outputFilename is gone, so there is nothing left to keep in sync and the maintenance concern goes away.
On the config field: fileNaming already exists on main, so I kept decoding it for backwards compatibility. It may be omitted or set to PathToUnderscores, and any other value is now a clear configuration error rather than a silently ignored setting, since dropping a user's explicit choice would be worse than telling them. The example target now builds two protos that share the name Duplicate.proto in different subdirectories, which only works because of the underscore naming and covers the original #1761 case. The PR description is updated to match.
|
For what it is worth on the drift concern: the new I am happy to follow whichever sharing strategy you land on. If you go with a copied source file plus a |
adityasingh2400
force-pushed
the
fix-1761-plugin-output-filenaming
branch
from
53555aa to
819de05
Compare
adityasingh2400
changed the title
|
@adityasingh2400 lets not worry about sharing the code immediately in this PR, it's something we can consider in the future. Swift gRPC has a close duplicate of this, and it would be nice to share that, but given the limitation that SwiftPM plugins can't have dependencies, there isn't an obvious good way to deal with it. 😦 Also, sorry for the delay here, with WWDC next week, Apple folks are little a little busy, and I've had a bunch of my plate, but I'll take a look now. |
| // derived the same way, so the names the build system expects can never drift from the | ||
| // names protoc-gen-swift actually writes. The configured fileNaming, if any, was already | ||
| // validated to be PathToUnderscores. | ||
| protocArgs.append("--swift_opt=FileNaming=\(Configuration.Invocation.FileNaming.pathToUnderscores.rawValue)") |
There was a problem hiding this comment.
If we are really just doing one mode, I'd likely just hardcode it so the option could eventually go away.
There was a problem hiding this comment.
Done in a9163dd, the protoc arg is now the hardcoded string so nothing references the FileNaming enum on this path anymore.
| } | ||
| // The build plugin always uses PathToUnderscores naming. Reject any other explicit | ||
| // value so the setting is never silently ignored. | ||
| if let fileNaming = invocation.fileNaming, fileNaming != .pathToUnderscores { |
There was a problem hiding this comment.
@glbrntt you all good with making this error if folks had another value set? Or should print a diagnostic of some kind about ignoring it?
There was a problem hiding this comment.
Rather than throwing an error we should emit a Diagnostic.warning() (I think that's the API) to avoid breaking builds.
There was a problem hiding this comment.
Done in 673b124. validateConfiguration now emits Diagnostics.warning saying the value is ignored and PathToUnderscores is always used, and the unsupportedFileNaming error case is gone. Existing configs keep building.
There was a problem hiding this comment.
The validation and diagnostic should come earlier: it's currently in the input file loop so users would see the diagnostic repeated N times. I think it should come just before --swift_opt=FileNaming=PathToUnderscores is added to the args.
|
Sounds good on deferring the code sharing, and no worries about the timing. I hardcoded the protoc option string per the inline note. For the existing fileNaming config values I will follow whichever way you and @glbrntt land, keeping the clear error or switching to a printed diagnostic that says the value is ignored. Both are a small change on my side. |
The SwiftProtobufPlugin build tool declared each generated file using the input proto's full relative path with the suffix swapped to .pb.swift, while passing the configured FileNaming straight through to protoc-gen-swift. When a target used PathToUnderscores or DropPath, or when two protos in different subdirectories shared a file name, the generator wrote a differently named file than the plugin declared and the build failed because the declared output did not exist. This is the problem reported in issue 1761. The generated files live in the build directory where their names are never observed, so there is no value in offering a file naming choice here. The plugin now always generates with PathToUnderscores and derives the declared output names with the same transformation, replacing the directory separators with underscores. Pinning both the generator invocation and the declared outputs to one strategy means they can never diverge, which also removes the need to mirror the generator's FileGenerator.outputFilename logic. The fileNaming config field already existed on main, so it is kept for backwards compatibility. It may be omitted or set to PathToUnderscores. Any other value is now a clear configuration error rather than a silently ignored setting. A PathToUnderscores example target builds two protos that share the file name Duplicate.proto in different subdirectories, which only succeeds because of the underscore naming, covering the case from issue 1761. Fixes apple#1761
Emit the ignored-fileNaming diagnostic once per invocation in invokeProtoc, just before the FileNaming=PathToUnderscores arg is added, instead of in the validateConfiguration loop. The behavior is unchanged, the warning just no longer risks being repeated per file.
adityasingh2400
force-pushed
the
fix-1761-plugin-output-filenaming
branch
from
673b124 to
620ff4b
Compare
|
Good call. I moved the validation and diagnostic out of the per-file validation loop. The ignored-fileNaming warning now fires once per invocation in invokeProtoc, right before the FileNaming=PathToUnderscores arg is added, so it can no longer be repeated N times. Behavior is otherwise unchanged. Also rebased on latest main. |
4 participants
The SwiftProtobufPlugin build tool plugin declared the path of every generated file by taking the input proto path and swapping the .proto suffix for .pb.swift while keeping the full relative path, and it passed the configured fileNaming straight through to protoc-gen-swift.
That breaks the build whenever the declared output name does not match what protoc-gen-swift actually writes. With PathToUnderscores or DropPath the generator writes a differently named file than the plugin declared, and two protos that share a file name in different subdirectories collide. The build system then fails because the declared output does not exist. This is the problem reported in #1761.
Per the discussion on this PR: the generated files go into the build directory, so the file name on disk is never observed and there is no value in giving users a choice for file naming when using the build plugin. The plugin now always uses PathToUnderscores, the same approach grpc-swift-protobuf takes.
What changed:
On the existing fileNaming config field: it already exists on main, so I kept decoding it for backwards compatibility. It may be omitted or set to PathToUnderscores. Any other value is now a clear configuration error rather than a silently ignored setting, since dropping a user's explicit choice on the floor would be worse.
Verification: added a PathToUnderscores example target whose two protos share the file name Duplicate.proto in different subdirectories. This only builds because of the underscore naming, which is the case from #1761. swift build and swift test --package-path PluginExamples pass, including the new testPathToUnderscores case, and the existing examples stay green under the new default. I also confirmed by hand that a DropPath or FullPath setting now produces the configuration error.
Fixes #1761