comtypes icon indicating copy to clipboard operation
comtypes copied to clipboard

Implement static import for `ISequentialStream` (#474)

Open jonschz opened this issue 1 year ago • 1 comments

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 avatar Jun 29 '24 06:06 jonschz

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".

junkmd avatar Jul 03 '24 13:07 junkmd

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.

jonschz avatar Jul 05 '24 05:07 jonschz

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 https://github.com/enthought/comtypes/issues/474#issue-1536002087, 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 avatar Jul 06 '24 01:07 junkmd

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.

junkmd avatar Jul 06 '24 01:07 junkmd

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!

jonschz avatar Jul 06 '24 09:07 jonschz