(try to) fix the protobuf file conflict voodoo

[don-de-marco]

No description provided.

[don-de-marco]

The diff looks awful becuse of the recompiled protobuf files, I'm sorry. With this change, however, the issue with the colliding message names and type names should be gone for good (given that Valve doesn't change it again). Yet, this needs some testing although it seems to me that everything still works perfectly fine.

[ABaumher]

I don't disagree that my solution sucked. The original version had compiled protobuf files that were in the auth group, but they've since been removed, so i don't have the proto source files. It's definitely smarter to not merge the stuff into a resolved proto. Renaming the invoke commands is definitely a good idea. Iirc i didn't just hack swap the includes because one of the service protos had a conflict where it required something in common_base that wasn't in steammessages_base.

I don't actually know if the proto2 header is required. The code for parsing the proto files was taken from ValvePython, but they still support python 2. Afaik protobufs version 3 don't officially support python 2, so the devs there forced it to work by treating the files as protobuf version 2. It might still be required because we're using a really old protoc, i'm not sure.

I've avoided protobuf 3.19 because it was compiling the python with a builder object, which did not exist. i think the google/protobuf whatever needs to be updated to use it, idk. Documentation for GOG plugin update seems ok.

ValvePython is in the middle of updating their code to do the new auth flow (we beat them to it lol), so they may remove or alter that proto. Might not be a good idea to reference it long term.

You'll need to update requirements/dev.txt to include requests or swap to urllib equivalent. I've been trying to keep our 3rd party imports to a minimal since they get packed alongside our plugin (that's what the old code did, too), but the dev ones shouldn't get packed and we're only using it to pull protoc which means it's limited to development so it's not really an issue.

[ABaumher]

realistically, we should use grpcio in addition to protoc (no idea how that works) because the modern proto files include the rpc message formats, which protoc will ignore without the rpc plugin. These can be used during the refactor to make our code simpler. They basically say "if you make the call foo, you need to supply message bar and you'll get message baz back" which means we could write some sort of send_message_and_wait function knowing what we will send and get. Since we do everything manually this would only be really useful for something like mypy but the type hinting with protos is a nightmare anyway so it might not be worth it

[ABaumher]

removing the temp file is probably good. i couldn't figure out how to get protos to properly compile unless they were all in the same directory.

Another minor fix, we probably want to fix the file paths for output dirs to be absolute paths. If you run inv from a different directory it might cause unexpected results because of relative paths. we can get the absolute path to the tasks.py file via script_dir = os.path.dirname(os.path.abspath(__file__)) #abspath for macos compatibility you already did that. mb

[ABaumher]

that needs to be changed since we're allowing mobile confirm now. I really need to code review this entire project, lol

[ABaumher]

importing the class made it easier to get the messages for whatever new call we were making or adding type hints to the existing code, but this is definitely more python-3 friendly. If we're done with development and adding type hints, this is probably a good idea.

[don-de-marco]

I mostly did this to make the rest of the file more readable by removing the extremely long module prefix from type hints etc. Also, this way one can see at a glance which messages we need in case there is a change in the proto files again.

[ABaumher]

I'm inclined to merge this, a little more cautious since last time i had to go find and revert a minor bug. the only thing holding me back atm is that valve python proto. i don't think they'll keep the file. If i'm being honest, all the protos scare me a bit, because they can be changed at any time, and calls may be moved to files we don't retrieve. That's what happened with that webui_friends proto and the call we make from it, and i had to clone and search steamdb using grep to find it.

Maybe we need to document what protobuf messages we use so if the protos do change in the future the new devs can search for them and update the include list based on what's there.

Imo the "best" solution might be to bring common and common base along, and hack include them in each of the steammessages_<whatever>. Then strip out any dupes from steammessages_base. but honestly, there isn't really a good solution. LMK what you think

[don-de-marco]

I don't disagree that my solution sucked. The original version had compiled protobuf files that were in the auth group, but they've since been removed, so i don't have the proto source files. It's definitely smarter to not merge the stuff into a resolved proto. Renaming the invoke commands is definitely a good idea. Iirc i didn't just hack swap the includes because one of the service protos had a conflict where it required something in common_base that wasn't in steammessages_base.

From what I saw both in my compiler errors logs and the commits in the steamdatabase repo, it seems that steammessages_unified_base is more or less identical to common+common_base in webui (see this commit).

I don't actually know if the proto2 header is required. The code for parsing the proto files was taken from ValvePython, but they still support python 2. Afaik protobufs version 3 don't officially support python 2, so the devs there forced it to work by treating the files as protobuf version 2. It might still be required because we're using a really old protoc, i'm not sure.

It is required, I looked it up. The extensions used by the proto files are only supported when using proto2 syntax. Both versions are current, though, but support different features so you must specify which one you'd like to use. See here for a comparison

I've avoided protobuf 3.19 because it was compiling the python with a builder object, which did not exist. i think the google/protobuf whatever needs to be updated to use it, idk. Documentation for GOG plugin update seems ok.

The generated protobufs worked for me, I did all my yesterday testing using those. I'm unsure why it didn't work for you earlier.

ValvePython is in the middle of updating their code to do the new auth flow (we beat them to it lol), so they may remove or alter that proto. Might not be a good idea to reference it long term.

After I committed everything I thought of pinning the commit hash in the download URLs so that we don't just download the ones from the master branch. That way we can ensure that we get proto files we want and would also solve the long-term issue (or postpone, to be precise; but in case an update of the protos is planned one must explicitly do that then by changing the commit hash to a more recent commit).

You'll need to update requirements/dev.txt to include requests or swap to urllib equivalent. I've been trying to keep our 3rd party imports to a minimal since they get packed alongside our plugin (that's what the old code did, too), but the dev ones shouldn't get packed and we're only using it to pull protoc which means it's limited to development so it's not really an issue.

For some reason, requests was installed in my venv so I assumed that it is a transitive dependency of some other package (requests-html maybe, but that's just guessing). If we keep requests to the dev dependencies we should be fine and I honestly don't mind just adding it there - those are dev deps after all. However, I agree with you that the run-time deps for the plugin should be kept as simple as managable.

[don-de-marco]

realistically, we should use grpcio in addition to protoc (no idea how that works) because the modern proto files include the rpc message formats, which protoc will ignore without the rpc plugin. These can be used during the refactor to make our code simpler. They basically say "if you make the call foo, you need to supply message bar and you'll get message baz back" which means we could write some sort of send_message_and_wait function knowing what we will send and get. Since we do everything manually this would only be really useful for something like mypy but the type hinting with protos is a nightmare anyway so it might not be worth it

I do not know how RPC interfaces in protobuf work. But I suspect that switching over to them requires a quite substantial amount of work on top of the other refactorings you are planning to do.

[don-de-marco]

Since I commented on all of these before, I just give bullet points here outlining what I suggested above.

I'm inclined to merge this, a little more cautious since last time i had to go find and revert a minor bug. the only thing holding me back atm is that valve python proto. i don't think they'll keep the file. If i'm being honest, all the protos scare me a bit, because they can be changed at any time, and calls may be moved to files we don't retrieve. That's what happened with that webui_friends proto and the call we make from it, and i had to clone and search steamdb using grep to find it.

using commit hashes instead of master in the download URLs should help with this (or at least avoid accidental conflicts)

Maybe we need to document what protobuf messages we use so if the protos do change in the future the new devs can search for them and update the include list based on what's there.

explicitly importing all the messages we need makes it clear to see what one needs to look for in case of changes

Imo the "best" solution might be to bring common and common base along, and hack include them in each of the steammessages_<whatever>. Then strip out any dupes from steammessages_base. but honestly, there isn't really a good solution. LMK what you think

steammessages_unified_base is more or less identical to common+common_base so I don't see the point in merging them manually and stripping dupes. Doing so will probably just create headaches and break every other day.

[ABaumher]

I do not know how RPC interfaces in protobuf work. But I suspect that switching over to them requires a quite substantial amount of work on top of the other refactorings you are planning to do.

yeah, fair. The only reason i'm considering it is the type hinting is atrocious and if the messages tell me what types i'm expecting, so much the better.

From what I saw both in my compiler errors logs and the commits in the steamdatabase repo, it seems that steammessages_unified_base is more or less identical to common+common_base in webui (see this commit).

Finding that commit is impressive. is there anything that's exclusive to steammessages_unified_base that we use? because we could just switch to common+common_base and hack include thos in place of unified base and that'd be (hopefully) future-proof

[don-de-marco]

I do not know how RPC interfaces in protobuf work. But I suspect that switching over to them requires a quite substantial amount of work on top of the other refactorings you are planning to do.

yeah, fair. The only reason i'm considering it is the type hinting is atrocious and if the messages tell me what types i'm expecting, so much the better.

From what I saw both in my compiler errors logs and the commits in the steamdatabase repo, it seems that steammessages_unified_base is more or less identical to common+common_base in webui (see this commit).

Finding that commit is impressive. is there anything that's exclusive to steammessages_unified_base that we use? because we could just switch to common+common_base and hack include thos in place of unified base and that'd be (hopefully) future-proof

The only reason I chose to use steammessages_unified_base was that I in this case only need to change the include of the webui files since all other steammessages_... files include steammessages_unified_base already (even those which don't need it :'D)

[ABaumher]

iirc the one you pull from ValvePython was replaced by service_community.proto and that one does require something in common+common base that's not in unified_base, which is why i'd lean toward common + common base. We're already dumping the entire file into a stream anyway to replace cc_generic_services with py_generic_services and fix steamclient.proto includes so if we alter it in 10 files vs 2 it's not really an issue.

EDIT: I could be wrong, let me check.

[ABaumher]

service community conflicts with unified base, it redefines a few things. that was the issue. let me see if we can get away with common+common base everywhere else.

Edit: If not, pin that commit from valve python, update the requirement list accordingly. We can just kick it down the line to someone else when steam finally breaks it. If we can get away with common+ common base, swap the hack to the steammessages. I'll merge after whichever fix we go with is implemented.

[ABaumher]

There are way too many conflicts. just pin it.

[ABaumher]

I've avoided protobuf 3.19 because it was compiling the python with a builder object, which did not exist. i think the google/protobuf whatever needs to be updated to use it, idk. Documentation for GOG plugin update seems ok.

The generated protobufs worked for me, I did all my yesterday testing using those. I'm unsure why it didn't work for you earlier.

What protoc were you using? because i think i was trying protoc 3.19.4 and that introduced builder which wasn't actually available in the protobuf package until 3.20, which adds a huge amount of breaking changes

[don-de-marco]

I've avoided protobuf 3.19 because it was compiling the python with a builder object, which did not exist. i think the google/protobuf whatever needs to be updated to use it, idk. Documentation for GOG plugin update seems ok.

The generated protobufs worked for me, I did all my yesterday testing using those. I'm unsure why it didn't work for you earlier.

What protoc were you using? because i think i was trying protoc 3.19.4 and that introduced builder which wasn't actually available in the protobuf package until 3.20, which adds a huge amount of breaking changes

Use inv InstallProtobuf or whatever I named that command. It installs a/the working copy of protoc into the source tree and also uses that one to compile all proto files

[ABaumher]

I have protoc in my environmental variables, but iirc windows looks in local directory first. If you specified a path to it, that's definitely the case. Looks fine to me. Do you have a static url for https://raw.githubusercontent.com/ValvePython/steam/master/protobufs/steammessages_webui_friends.proto ? better question - is there a specific commit in steam database that has that file so we can pull from there?

[don-de-marco]

I have protoc in my environmental variables, but iirc windows looks in local directory first. If you specified a path to it, that's definitely the case. Looks fine to me. Do you have a static url for https://raw.githubusercontent.com/ValvePython/steam/master/protobufs/steammessages_webui_friends.proto ? better question - is there a specific commit in steam database that has that file so we can pull from there?

I use an absolute path to run protoc so it doesn't matter which version you have installed locally or where it is.

You can get the commit link by navigating the source tree on Github and then pressing y: https://raw.githubusercontent.com/ValvePython/steam/26166e047b66a7be10bdf3c90e2e14de9283ab5a/protobufs/steammessages_webui_friends.proto