-
Notifications
You must be signed in to change notification settings - Fork 844
Fix struct handling in makepy generated code #2560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@mhammond - that looks like a flaky test, as I did not change anything in that area. can s/b please verify? |
unfortunately one more issue regarding same guids in different typelibs --- meh |
activating again; a problem exists for VT_RECORD without VT_BYREF; most probably that will not work through IDispatch -- at least I tried enough and don't want to continue on that we will change our server to VT_BYREF... |
The datetime failure is indeed a flaky test (ref: #2203) |
com/win32com/client/build.py
Outdated
and desc[3] | ||
and desc[3].__class__.__name__ == "PyIID" | ||
): | ||
newVal = f"pythoncom.GetRecordFromGuids(CLSID, MajorVersion, MinorVersion, LCID, {repr(desc[3])})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @geppi, do you have any thoughts on this pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I'm in the mountains over the long Easter weekend and can take a detailed look into this earliest on tuesday. My first thought is that I should probably just write some documentation about the functionality we recently introduced for COM Records with PR #2437.
However, I'm not sure where this documentation should live and what format would be required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like in-code comments parsed by a tool called AutoDuck is used to generate helpfiles. Those helpfiles are then decompiled to extract the HTML and are hosted as a GitHub page:
In #2432 I'm trying to restore shipping the helpfiles (by adding them to the build commands without building pywin32 twice) to users for local no-internet documentation.
In #2577 I added a link to a mirror of AutoDuck's download which included documentation and examples on how to use it. Hopefully that should help with its usage.
review
Co-authored-by: Avasam <[email protected]>
Co-authored-by: Avasam <[email protected]>
@Avasam I wonder why the tests still fail, though the encoding changes are reverted |
The problem with this PR is that it doesn't create proper Python classes:
Ooops! I've opened PR #2578 to add documentation for the COM Record support including the recent improvement of PR #2437. |
@geppi - unfortunately, the mechanism that you describe does not work with makepy generated code (see the unit test part in this pr) meaning: this was the smallest possible change in my opinion; of course, a larger change is possible where proper Python records are used. |
I'm afraid I don't understand the problem.
Could you please provide an example what fails? Also if you follow the recipe to create subclasses of |
We are using code generated by makepy, so actually the generated code shall contain all necessary constructors (in my opinion). Have a look at the IDL changes and the tests within this PR. What actually does not work (without modification in build.py) is:
The default argument results in a The other problem we experienced is not that easy to reproduce right now with that minimal test. Consider this:
In this scenario, a file is generated by gencache (which is actually not necessary, since everything should be already present in the file that has been generated with makepy). In our production environment, it happened that this file could not be read afterwards and we ended up with an import error. As said, not reproducible within this small test right now. |
I debugged that in our scenario:
In the end, no Python file is generated and the import fails. My temp directory contains the following files in gen_py\3.13: Nothing more. @geppi --- how to proceed? |
I would suggest to fix the code that creates the classes for the records as a first step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formally marking as "request changes" as per the existing comments.
In code generated by makepy, there was no chance to create struct instances, as using Record failed. Convenience constructors are generated now.
In addition, functions using structs as out values could not be called, as the default (missing) value could not be converted to a struct instance. The change creates struct instances automatically in such cases.