grpc-proto
grpc-proto copied to clipboard
grpc
Add routeguide/route_guide.proto
@chalin doing this means we should also delete it from the grpc-go repo, which could unfortunately complicate the quick-start guide. What do you think?
Also, we should diff this with the Java and C copies and make sure they are currently identical or close enough, too. Not sure if those repos have a process to ensure their proto copies are up-to-date, but if so, they should start checking this file, too.
@chalin doing this means we should also delete it from the grpc-go repo, which could unfortunately complicate the quick-start guide. What do you think?
Actually, no. As is mentioned in the README:
For ease of development, proto files will still be copied to the other gRPC repositories (e.g.,
grpc/grpc,grpc/grpc-go, etc.).
Also, we should diff this with the Java and C copies and make sure they are currently identical or close enough, too.
Yes, I've been doing that. As @ejona86 requested in https://github.com/grpc/grpc-proto/pull/80#pullrequestreview-401306624:
Let's leave this PR "pristine". Changes to it can be follow-up.
So this is a copy of the grpc-go version, which I plan to subsequently update to ensure that it contains the options that are present in all language-specific variants.
How does that sound?
Not sure if those repos have a process to ensure their proto copies are up-to-date, but if so, they should start checking this file, too.
Agreed, each repo should eventually make the necessary checks to ensure that local proto files match their canonical sources.
Actually, no. As is mentioned in the README:
We don't currently copy any proto files to grpc-go from this repo; we deleted our copies when they were added here. I'm fine with making things a copy for the purposes of documentation, but then we'd need a new special test to make sure the copied file is up-to-date.
Do we have a guide that references route_guide.proto? If not, we should delete it from grpc-go and update the generation script to produce it from grpc-proto.
How does that sound?
SG.
Agreed, each repo should eventually make the necessary checks to ensure that local proto files match their canonical sources.
If they don't currently have these sanity checks for other proto files, that should be a pretty high priority thing to do IMO. @markdroth @ejona86 ?
I don't think we have any sanity checks for this today. I agree that we should fix that, but I don't think this is a very high priority. If all of our interop tests are working, I don't think it matters if the files are exactly identical down to each byte.
grpc/examples/route_guide.proto
Darn. I missed that in #80. Neither of these example protos are in the correct directory. They should be in a directory that matches their name (so "helloworld" and "routeguide"). Since we aren't changing the package names to be grpc.examples.*, we should keep the folders at the top-level as well.
If they don't currently have these sanity checks for other proto files, that should be a pretty high priority thing to do IMO. @markdroth @ejona86 ?
When we do an update of any proto, we do a "full" update of all the protos. https://github.com/grpc/grpc-java/blob/master/buildscripts/sync-protos.sh
I don't think anyone should have a "sanity check" that would fail if the proto is no longer up-to-date. It is fine to use an older version. If you checked it against a particular SHA, that would be fine. For Java, we just routinely re-copy the files.
I don't think anyone should have a "sanity check" that would fail if the proto is no longer up-to-date. It is fine to use an older version. If you checked it against a particular SHA, that would be fine. For Java, we just routinely re-copy the files.
I disagree, as does this comment in the README.md in this repo:
Sanity tests will be added to verify that common proto files match the "ground truth" files contained here.
Having multiple copies of anything is a bad idea in the first place, but if we must do that, then we need to make sure the copies are kept up-to-date and, more importantly, not accidentally modified in place.
Sanity tests will be added to verify that common proto files match the "ground truth" files contained here.
I see the "ground truth" there as that they match at some commit.
Having multiple copies of anything is a bad idea in the first place, but if we must do that, then we need to make sure the copies are kept up-to-date and, more importantly, not accidentally modified in place.
It is 100% okay for protos to be out-of-date. That is how they work. Making a build go red each time a misspelling is fixed is obnoxious and harmful. But I agree it would be unfortunate if a proto is a year old and so it takes ~forever to get documentation fixes.
I completely agree that we don't want protos modified in-place. I believe that expected protection against that is for us to use //third_party/grpc_proto internally. I believe there is still work to do there, mainly for the C repo. Grpc-java also re-copies all the appropriate protos so that no single proto gets too old.
It is 100% okay for protos to be out-of-date. That is how they work.
Yes and no. It's fine for a local cache to be out of date, but when our users see something called service_config.proto in an official grpc repo, they may think it's the canonical version. If it's out of date, then we have done them a disservice.
Maybe if you add a comment to the top of the cached files to point to the canonical version, then it would be OK if it drifts slightly.
Making a build go red each time a misspelling is fixed is obnoxious and harmful.
We only fail our nightly runs only when things are out of date. It's a reminder to update our generated code and doesn't block commits. This seems fine. The canonical protos are not updated very frequently.
I completely agree that we don't want protos modified in-place. I believe that expected protection against that is for us to use //third_party/grpc_proto internally.
This is too late in the process to catch these problems. We should have protections in github, and ones that are directly intended to catch this problem, not a side-effect of eventual sharing that happens elsewhere.
We only fail our nightly runs only when things are out of date.
What nightly runs? From what I've seen, nobody is watching the master builds. We only notice problems when PRs are impacted.
Maybe if you add a comment to the top of the cached files to point to the canonical version, then it would be OK if it drifts slightly.
We were planning to do that. That was mentioned in the initial description of #80, for example, to discuss whether it should be a separate PR or not. Some of the protos in grpc-proto already have that, but I don't think that's universal.
The canonical protos are not updated very frequently.
Each individual file isn't updated frequently. As we have more, the frequency that any one of them changes increases. For this year (which I agree is choosing data a bit), we've seen a change on average every 10 days. Now, there are spikes. But I will be quite annoyed if we need a syncing PR every other week to pick up changes rapidly.
This is too late in the process to catch these problems.
"too late" Hmm.. I don't think I agree. C imports every day. Java imports twice a week. Difference between "nightly" and "2-3 days" doesn't seem major.
We should have protections in github, and ones that are directly intended to catch this problem, not a side-effect of eventual sharing that happens elsewhere.
Then I would suggest having a check in Go that the files match a particular commit of this repo. That soundly resolves the local mutation concern. If you want a nightly run on Go that will email you when things are out-of-sync, that's fine. But I don't want it for Java.
grpc/examples/route_guide.proto
Darn. I missed that in #80. Neither of these example protos are in the correct directory. They should be in a directory that matches their name (so "helloworld" and "routeguide"). Since we aren't changing the package names to be grpc.examples.*, we should keep the folders at the top-level as well.
Since we aren't suggesting that folks compile these two particular proto files in-place, then it should be ok to have these protos reside at the end of a meaningful path. Besides, each language will be free to place copies of either of these simple example protos in a folder of their choosing. More importantly, they are free to place the protoc output in a folder of their choosing.
But if the convention that we're trying to enforce in this repo, is that all proto file sources will be found at the end of a path that matches the proto file package name, then yes, both the hello world and the route guide proto files will need to be at the top level under a folder matching the package name.
@ejona86 - Let me know if you'd like for me to relocate the route guide proto file in this PR.
For this year (which I agree is choosing data a bit), we've seen a change on average every 10 days. Now, there are spikes.
I guess I don't see a problem with needing to run a command every 10 days and click some buttons to keep your local cache fresh. And, yes, this spike is unusual due to RLS being new (and these examples being moved). Other proto files were changed in 4 PRs, so ~1 per month.
If you want to add a "canonical copy" pointer to every .proto file here and update the README to remove the line about all repos having a test, then I'm OK if your copies drift. We'll still make sure grpc-go's are updated. It seems problematic if we are behind, given that we are the canonical location for generated code for many of them.
@chalin I agree with your assessment; given that we want to change the proto package names, but can't for backward-compatibility reasons, we should use the directory structure that 1. is nicer for file organization, and 2. matches our intent.
(One caveat is we should add a comment above the package line explaining the package naming conundrum.)
But if the convention that we're trying to enforce in this repo, is that all proto file sources will be found at the end of a path that matches the proto file package name
The thing we want to enforce is that the proto package should match the folder path, so that imports in proto use the identical structure for both. That should be true everywhere.
It seems I disagree with @dfawley on this point.
What happens in places like grpc-go and grpc-java is that the proto root is not the top of the repository. In these cases protoc is invoked from subdirectories, or is passed subdirectories via -I (this is how the gradle-protobuf-plugin works; the project/src/main/proto and similiar directories will be the root, as that is "the Gradle way"). I will note that in the generated artifacts of grpc-java, the protos have the proper paths. But that doesn't matter at all to the .proto itself. So I'd say in this repo that we'd enforce that the repo root is the proto root.
(And that's partly because I haven't successfully gotten Bazel to work with other roots in all cases. I think it may be able to work now, at least in some languages, but we do horrible hacks some places to workaround old issues. In the past it was the grpc plugin+Bazel that wasn't working. Long story.)
It seems problematic if we are behind, given that we are the canonical location for generated code for many of them.
That's a fair concern, and I have zero problem with grpc-go syncing more rapidly. grpc-java is the canonical generated code for some as well. They just get updated frequently enough "naturally" it doesn't concern me, especially given there is little community usage of the protos and most of the changes seem to be in comments and similar non-functional changes. If it became a problem, I'd add the syncing to our release process (at the time of the branch cut, which y'all don't have).
The agreement between the directory and the package, and the directory being at the top-level is (too-)briefly described at https://github.com/grpc/grpc-proto#directory-structure
- Moved proto into root-level
routeguidefolder, matching the proto's package name. - Added "The canonical version of this proto ..." message.
@ejona86 @dfawley PTAL
I'm still opposed to having a top-level routeguide directory. It clutters the repo in confusing ways because of a mistake made years ago. I think we should put it in examples/routeguide, and also have examples/helloworld. Here, examples would be considered the root directory of a separate proto tree.
I think we're at an impasse. We can't currently have a separate proto root with Bazel.
We can't currently have a separate proto root with Bazel.
Ok, then that rules out having an examples folder.
I'm still opposed to having a top-level
routeguidedirectory. It clutters the repo in confusing ways because of a mistake made years ago.
Considering the principles that we are trying to enforce in this repo, IMHO the "cost" of the clutter is minor and it is a consequence of maintaining backwards compatibility "forever" when it comes to proto files.
I think we're at an impasse. We can't currently have a separate proto root with Bazel.
"Can't" or "it's hard" or "you don't know how"?
Can we move the BUILD.bazel in the root directory under grpc?
"Can't" or "it's hard" or "you don't know how"?
"All grpc protoc plugins except for Java and Go are likely incompatible." It is a LANG_grpc_library() issue, but fixing those are not a trivial undertaking, and may not yet be possible in every language.
Can we move the BUILD.bazel in the root directory under grpc?
No. The only workaround I know to maintain the proto root is to add a WORKSPACE file to examples/ directory. This would mean users would need to include two "separate" http_archives: grpc-proto and grpc-proto/examples. If you accidentally used @grpc-proto//examples instead of @grpc-proto-examples// you would get strange errors.
The LANG_grpc_library()s may actually support proto roots that don't match the WORKSPACE root. I see some generic code to manage it. But I don't see any tests. Since it seems someone has at least thought about it, maybe we could use strip_import_prefix here and have things work, in Bazel. If things don't actually work then languages will have to fix their rules (which isn't bad).
But each build system has to be similarly adapted. We don't have that sort of control in Java with Gradle; we'd probably need to "manually" restructure the folders to match the proto package, which would mean special-case 'sed's in our import script. (Note that when you get this wrong there can be subtle problems, like reflection producing different results. Probably not too big of a deal, since this file is not imported, but I also don't feel like the gain is worth the risk.)
@chalin, in talking with @dfawley, he's said he doesn't really care any more. It seems it was a battle of attrition, which doesn't make me feel great, but is where we're at.
That would mean we'd go with the "make the root directory ugly" approach, with a comment above the proto package name that ideally it would have been named "grpc.examples.routeguide" and be in the "grpc/examples/routeguide/" directory.
Even if we can get strip_import_prefix working in all build systems in all languages, it is very strange to have one root inside another and would complicate the example. Ugly is better than complex, in this case. I wish we could fix the package, but as we've discussed it seems like that would noticeably harm new users for limited gain.
It seems there are technical conflicts between bazel and a well-organized directory structure. Since I have neither the time nor willpower to personally solve or workaround the bazel issues, I agree to let the structure of our repository suffer as a consequence.