Skip to content
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

Implement static import for ISequentialStream (#474) #578

Merged
merged 8 commits into from
Jul 6, 2024

Conversation

jonschz
Copy link
Contributor

@jonschz jonschz commented Jun 29, 2024

See #474. I migrated the code from the old istream-issue branch to the latest main branch. Let me know if anything else needs to be done.

Closes #474

@jonschz jonschz changed the title Implement static import for ISequentialStream (#529) Implement static import for ISequentialStream (#474) Jun 29, 2024
@junkmd junkmd added the enhancement New feature or request label Jun 30, 2024
Copy link
Collaborator

@junkmd junkmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the parts of the test that I was concerned about.

There are no problems with the production codebase or its behavior.

As remaining tasks to merge this PR, we need to work on naming and writing comments.

comtypes/objidl.py Outdated Show resolved Hide resolved
comtypes/objidl.py Outdated Show resolved Hide resolved
comtypes/objidl.py Outdated Show resolved Hide resolved
junkmd added a commit to junkmd/pywinauto that referenced this pull request Jun 30, 2024
junkmd added a commit to junkmd/pywinauto that referenced this pull request Jun 30, 2024
@junkmd
Copy link
Collaborator

junkmd commented Jul 3, 2024

I have come across a repository that makes RemoteRead usable by directly modifying the module generated in comtypes.gen (such as changing pv to ["in", "out"]). If this PR is merged, that approach will be no more, and that codebase may not work.

However, that approach involves modifying the codebase within the comtypes package. In other words, it is essentially synonymous with forking and applying a custom patch for use.

Therefore, I think that those codebases becoming inoperable due to this change does not constitute "breaking backward compatibility".

@jonschz
Copy link
Contributor Author

jonschz commented Jul 5, 2024

In other words, it is essentially synonymous with forking and applying a custom patch for use.

I agree with that. If all the downstream code we break is code that changed this packages behaviour, that is not a "breaking change" in my opinion, since they did not use part of our official API. Giving them a heads up won't hurt, but whenever you do something like that, you pretty much have to do version pinning (or blame yourself if an external update breaks your package).

The part of the official API that we did change has never worked in the past to begin with, so it's quite a stretch to call that a "breaking change" in my opinion.

@junkmd
Copy link
Collaborator

junkmd commented Jul 6, 2024

Giving them a heads up won't hurt, but whenever you do something like that, you pretty much have to do version pinning (or blame yourself if an external update breaks your package).

I am planning to include this change in the release of version 1.4.5.
This means that we will explicitly state that the 'custom patch' approach to RemoteRead under comtypes.gen can only be used up to version 1.4.4.

For example, if there is a 'custom patch' version of RemoteRead that

sets the first parameter to be ['in', 'out']

as you mentioned in #474 (comment), it is necessary to make the following changes:

buffer_size = 1024
- pv = (c_ubyte * buffer_size)()
- read_buffer, data_read = stream.RemoteRead(pv, buffer_size)
+ read_buffer, data_read = stream.RemoteRead(buffer_size)

Since it is impossible for us to investigate how widely the 'custom patch' is used in the world, I do NOT plan to find and go around individual repositories to encourage caution.

The part of the official API that we did change has never worked in the past to begin with, so it's quite a stretch to call that a "breaking change" in my opinion.

I agree with this.
I think making something that was causing some kind of error at the time of the call usable is in the realm of 'bug fixes'.
I do NOT think it's a "breaking change" if a user's ad hoc workaround starts causing errors when a bug has been fixed upstream.

I will also mention that this change only affects ISequentialStream defined within the module generated by GetModule in comtypes.gen, and does not break codebases that define and implement ISequentialStream (and tagSTATSTG and IStream) statically on their own.

@junkmd
Copy link
Collaborator

junkmd commented Jul 6, 2024

I recognize that there are (other than ISequentialStream's RemoteRead) methods that must be overriden because the codebase generated by codegenerator is not usable as is.
However, in this project, I am reluctant to statically define those interfaces and methods anything and everything.
Further discussion is needed in a separate scope from #474 for interfaces other than ISequentialStream.

I am considering accepting #578 is because IStream is the de facto standard for handling streams in COM, and I think it is in the community's profit to make RemoteRead usable.

comtypes/client/_generate.py Outdated Show resolved Hide resolved
comtypes/objidl.py Outdated Show resolved Hide resolved
junkmd added a commit to junkmd/pywinauto that referenced this pull request Jul 6, 2024
@junkmd junkmd added this to the 1.4.5 milestone Jul 6, 2024
@junkmd junkmd merged commit b3a5ed7 into enthought:main Jul 6, 2024
49 checks passed
@jonschz
Copy link
Contributor Author

jonschz commented Jul 6, 2024

I recognize that there are (other than ISequentialStream's RemoteRead) methods that must be overriden because the codebase generated by codegenerator is not usable as is.

I am sure that other examples will show up over time. As I don't see an automatic fix at the moment, I suggest we just implement them by hand as they come in. The fact that ISequentialStream is (afaik) the only one reported so far suggests that the problem is not extremely frequent.

Thank you for your efforts and contributions to this project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning out arguments breaks when they must be preallocated
2 participants