search

enthought / comtypes

A pure Python, lightweight COM client and server framework, based on the ctypes Python FFI package.
Other
290 stars 96 forks source link

Implement static import for `ISequentialStream` (#474) #578

Closed jonschz closed 3 months ago

jonschz commented 3 months ago

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

junkmd commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

jonschz commented 3 months ago

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!