rules_swift icon indicating copy to clipboard operation
rules_swift copied to clipboard

[swift_proto_library]filenames are used to distinguish private declarations with the same name

Open zhungxd opened this issue 4 years ago • 2 comments

i have proto file: path: app/a/api.proto path: app/b/api.proto

with [swift_proto_library] ,It fails :0: note: filenames are used to distinguish private declarations with the same name :0: error: filename "api.pb.swift" used twice: 'bazel-out/darwin-fastbuild/bin/external/xxxx/app/a/api.pb.swift' and 'bazel-out/darwin-fastbuild/bin/external/xxxx/app/b/api.pb.swift'

Can we rename the file to app_a_api.pb.swift 、app_b_api.pb.swift ?

zhungxd avatar Jan 19 '21 07:01 zhungxd

I don't know the full background on this, maybe @allevato can provide some context

keith avatar Oct 26 '21 18:10 keith

To get this situation, it sounds like app/a/api.proto and app/b/api.proto are both in the same proto_library target?

That's not a recommended code organization for .proto files (though unfortunately the proto_* rules don't do anything to enforce it). See https://docs.bazel.build/versions/main/be/protocol-buffer.html#proto_library, where they recommend a single .proto file per proto_library.

At the very least, it's unusual (and not recommended) to have a single proto_library that contains .proto files spanning multiple directories.

It's likely that such an organization would cause problems with other LANG_proto_library implementations as well; I suggest breaking up the proto_library.

allevato avatar Oct 26 '21 18:10 allevato

Is there any particular reason for not using --swift_opt=FileNaming=PathToUnderscores in this case as it's (probably) more portable than --swift_opt=FileNaming=FullPath when used in the downstream swift_proto_library case?

https://github.com/bazelbuild/rules_swift/blob/225180bf8ac2b2f4cbda77dd1ef07a407aea4231/swift/internal/swift_protoc_gen_aspect.bzl#L174

mattrobmattrob avatar Oct 03 '22 22:10 mattrobmattrob

Drive by (as the part of the original authors) –

What makes you say PathToUnderscores is "more portable"?

A big problem with PathToUnderscores is collisions:

  • foo/bar/user.protofoo_bar_user.pb.swift
  • foo/bar_user.protofoo_bar_user.pb.swift
  • foo_bar/user.protofoo_bar_user.pb.swift

Sadly, happens more often than one might think.

thomasvl avatar Oct 03 '22 23:10 thomasvl

In this case, just trying to think about portability of the source for the downstream underlying swift_proto_library. Since --swift_opt=FileNaming=FullPath + foo/bar/user.proto & foo_bar/user.proto will fail when used in the underlying swift_proto_library Swift module compilation despite the proto_library having no issues.

mattrobmattrob avatar Oct 03 '22 23:10 mattrobmattrob

What makes you say PathToUnderscores is "more portable"?

In my mind providing the option to choose FileNaming is far more valuable, not simply choosing FullPath or PathToUnderscores and demanding all developers follow a particular style guide since one size does not fit all. I'm running into this issue on a project and the only situation this is an issue is when building for Swift through Bazel. Every other build pipeline in this project is able to handle the schema and generate appropriate language bindings (including: Typescript, Java, Kotlin, Golang).

A big problem with PathToUnderscores is collisions

It sounds like Swift is just a poor target for Protobuf, and while allowing PathToUnderscores would be a (very helpful) bandage to the problem, perhaps additional .proto options need to be introduced similar to Java's option java_outer_classname = "Foo"; which is used in a similar way to avoid naming collisions.

ObsidianX avatar Oct 12 '22 19:10 ObsidianX

Just for reference, I believe the cc compiling apis (and cc_library and objc_library), deal with the collisions by ensuring the .o files all get unique names. I'm not sure if the swift compiling apis could get that sorta fix. If they could, not only would it fix this issue, it would also fix any place folks have authored files with the same basename but in different directories where they end up in the same swift_library, right now those folks have no option but to split them into different libraries. Folks like hit that less because Xcode can't deal with it either.

thomasvl avatar Oct 12 '22 20:10 thomasvl

Just for reference, I believe the cc compiling apis (and cc_library and objc_library), deal with the collisions by ensuring the .o files all get unique names. I'm not sure if the swift compiling apis could get that sorta fix.

This isn't possible for Swift (it's a compiler limitation, not a Bazel rules ones): no two source files in the same module can have the same basename, even if they're in different directories, because the hash of the basename is used as a discriminator for private symbols.

allevato avatar Oct 12 '22 20:10 allevato