-
Notifications
You must be signed in to change notification settings - Fork 129
Order of entries in generated 'enums' is not stable #2319
Comments
Another example today: https://github.com/GoogleCloudPlatform/google-cloud-python/pull/6109/files |
@JustinBeckwith, @theacodes per today's call. |
@lukesneeringer @andreamlin any more information we can provide here? |
Any update here? |
Hey folks, one question: Is the order non-deterministic or did the order remain deterministic and change? I think this change may have been caused by some work done to sort other components (not specific to Python). |
Thanks for reporting this. Forewarning: I'm not a Python expert.
|
Talked to Luke offline.
|
@vam-google Would https://github.com/googleapis/googleapis/blame/master/google/bigtable/admin/v2/BUILD.bazel have to do with this? Since that explicitly lists out the order in which the proto files are given to gapic-generator. If so, I think we can close this issue; the new Bazel build files will deterministically order the proto files. This could result in more no-op library refreshes down the road as they're being rolled out. |
Would it be possible to update this specific Bazel build file to match what was in the legacy configuration (which would theoretically revert the enum sort order in Bigtable)? |
Talked to @vam-google and it can't be related because that BUILD file was pushed after this issue was filed. I've been looking at how we generate the enum classes, and it seems to come from the order the proto files are passed in. This seems like a deterministic order. However, as Luke mentioned, we have been going through some refactorings that change the the order of surface entities in the clients. This churn should settle down in a while. |
This is not a case of one-time changes: I've seen multiple autosynth-driven PRs which do nothing more than change enum order: approving one doesn't make the problem go away. When I run synthtool on my machine, I have gotten a different order than the one in the autosynth PR. Note that I don't really care about any specific order, only that it be stable over time. |
@andreamlin I don't know how to tell if the change from #2418 has actually been rolled out to the artman image: is there some way to check locally using synthtool? |
I believe this issue is now closed, although, like Ahrnuld, "I'll be back!" if it pops up again in future autosynth runs. |
Sigh. |
New incidences today: |
@andreamlin Is there a way we could just sort the top-level enums / messages by name, and sort the enums within messages by name? I can't see a way to get that done, looking at |
@tseaver Sure, if we're fine with a one-time change to the surface that orders all existing enums. I can give it a shot |
@tseaver See #2453 for a fix and googleapis/google-cloud-python#6707 for an example of what happens after that PR is applied. |
E.g., see: https://github.com/GoogleCloudPlatform/google-cloud-python/pull/6015/files.
The order should match the order in the
.proto
file, except that they might be collected from more than one: if that case in general, then sort them.The text was updated successfully, but these errors were encountered: