[ Umbrella Issue ] Cleanup remaining DeprecatedFileSystem users
Hi, I created the following: https://pharo.manuscript.com/f/cases/21858/Cleanup-remaining-DeprecatedFileS... This is an umbrella issue, please create sub issues for each set of changes. It is better to work step by step. The package DeprecatedFileSystem contains classes that should no longer be used, the most important being FileStream, StandardFileStream and MultiByteFileStream MultiByteBinaryOrTextStream and RWBinaryOrTextStream Alternatives exist by using FileSystem's FileReference as well as File and various new (Zn) streams. Let's all work together to make this real. It is important that the image itself makes correct use of ongoing refactoring efforts. We already took great steps, but there is more work to do. Note that in many cases, using code can and should be simplified, removing ugly hacks where possible. Feel free to discuss any issues on the ML. Sven
Hi Sven, in that process, how does one rewrite StandardFilestream subclasses? Regards, Thierry 2018-05-09 11:51 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
Hi,
I created the following:
https://pharo.manuscript.com/f/cases/21858/Cleanup-remaining-DeprecatedFileS...
This is an umbrella issue, please create sub issues for each set of changes. It is better to work step by step.
The package DeprecatedFileSystem contains classes that should no longer be used, the most important being
FileStream, StandardFileStream and MultiByteFileStream MultiByteBinaryOrTextStream and RWBinaryOrTextStream
Alternatives exist by using FileSystem's FileReference as well as File and various new (Zn) streams.
Let's all work together to make this real. It is important that the image itself makes correct use of ongoing refactoring efforts. We already took great steps, but there is more work to do. Note that in many cases, using code can and should be simplified, removing ugly hacks where possible.
Feel free to discuss any issues on the ML.
Sven
You mean you made a subclass of StandardFilestream yourself ? If yes, can you describe briefly why you did so ?
On 9 May 2018, at 13:12, Thierry Goubier <thierry.goubier@gmail.com> wrote:
Hi Sven,
in that process, how does one rewrite StandardFilestream subclasses?
Regards,
Thierry
2018-05-09 11:51 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
Hi,
I created the following:
https://pharo.manuscript.com/f/cases/21858/Cleanup-remaining-DeprecatedFileS...
This is an umbrella issue, please create sub issues for each set of changes. It is better to work step by step.
The package DeprecatedFileSystem contains classes that should no longer be used, the most important being
FileStream, StandardFileStream and MultiByteFileStream MultiByteBinaryOrTextStream and RWBinaryOrTextStream
Alternatives exist by using FileSystem's FileReference as well as File and various new (Zn) streams.
Let's all work together to make this real. It is important that the image itself makes correct use of ongoing refactoring efforts. We already took great steps, but there is more work to do. Note that in many cases, using code can and should be simplified, removing ugly hacks where possible.
Feel free to discuss any issues on the ML.
Sven
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top. Regards, Thierry
On 9 May 2018, at 13:12, Thierry Goubier <thierry.goubier@gmail.com> wrote:
Hi Sven,
in that process, how does one rewrite StandardFilestream subclasses?
Regards,
Thierry
2018-05-09 11:51 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
Hi,
I created the following:
https://pharo.manuscript.com/f/cases/21858/Cleanup-remaining-DeprecatedFileS...
This is an umbrella issue, please create sub issues for each set of changes. It is better to work step by step.
The package DeprecatedFileSystem contains classes that should no longer be used, the most important being
FileStream, StandardFileStream and MultiByteFileStream MultiByteBinaryOrTextStream and RWBinaryOrTextStream
Alternatives exist by using FileSystem's FileReference as well as File and various new (Zn) streams.
Let's all work together to make this real. It is important that the image itself makes correct use of ongoing refactoring efforts. We already took great steps, but there is more work to do. Note that in many cases, using code can and should be simplified, removing ugly hacks where possible.
Feel free to discuss any issues on the ML.
Sven
On 9 May 2018, at 13:41, Thierry Goubier <thierry.goubier@gmail.com> wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
Yes, they will have to be ported. If it were me (but I haven't looked at the code) I would not inherit from those classes, but from a simpler Stream class, possibly even none. The stream API is actually quite small (or it should be). Inheritance is an implementation technique, inheriting from a complex class can be hard, unless the superclass was really designed with that purpose. All this, IMHO.
Regards,
Thierry
On 9 May 2018, at 13:12, Thierry Goubier <thierry.goubier@gmail.com> wrote:
Hi Sven,
in that process, how does one rewrite StandardFilestream subclasses?
Regards,
Thierry
2018-05-09 11:51 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
Hi,
I created the following:
https://pharo.manuscript.com/f/cases/21858/Cleanup-remaining-DeprecatedFileS...
This is an umbrella issue, please create sub issues for each set of changes. It is better to work step by step.
The package DeprecatedFileSystem contains classes that should no longer be used, the most important being
FileStream, StandardFileStream and MultiByteFileStream MultiByteBinaryOrTextStream and RWBinaryOrTextStream
Alternatives exist by using FileSystem's FileReference as well as File and various new (Zn) streams.
Let's all work together to make this real. It is important that the image itself makes correct use of ongoing refactoring efforts. We already took great steps, but there is more work to do. Note that in many cases, using code can and should be simplified, removing ugly hacks where possible.
Feel free to discuss any issues on the ML.
Sven
On Wed., 9 May 2018, 13:49 Sven Van Caekenberghe, <sven@stfx.eu> wrote:
On 9 May 2018, at 13:41, Thierry Goubier <thierry.goubier@gmail.com> wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
Yes, they will have to be ported.
If it were me (but I haven't looked at the code) I would not inherit from those classes, but from a simpler Stream class, possibly even none. The stream API is actually quite small (or it should be).
Inheritance is an implementation technique, inheriting from a complex class can be hard, unless the superclass was really designed with that purpose.
All this, IMHO.
I agree that these classes will need to be ported, just a little food for thought: I believe OSProcess is currently common between Squeak and Pharo - this porting will require that OSProcess is forked for Pharo, so the two implementations will start to diverge unless someone actively keeps them in sync. (Please don't read this as a negative judgement, it is just a statement of my understanding of the situation). There are a number of plugin primitives that Dave added to OSProcess because FilePlugin didn't meet his requirements. Some of the recent changes made to FilePlugin reduce the need for additional primitives (it is possible to connect to an existing file or file descriptor, #atEnd works for pipes), and I think there are additional enhancements that can be made which would reduce the number even further (mark files as non-blocking) (but I haven't looked in to it properly and would want to get Dave's input first). Cheers, Alistair
On 9 May 2018, at 16:06, Alistair Grant <akgrant0710@gmail.com> wrote:
On Wed., 9 May 2018, 13:49 Sven Van Caekenberghe, <sven@stfx.eu> wrote:
On 9 May 2018, at 13:41, Thierry Goubier <thierry.goubier@gmail.com> wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
Yes, they will have to be ported.
If it were me (but I haven't looked at the code) I would not inherit from those classes, but from a simpler Stream class, possibly even none. The stream API is actually quite small (or it should be).
Inheritance is an implementation technique, inheriting from a complex class can be hard, unless the superclass was really designed with that purpose.
All this, IMHO.
I agree that these classes will need to be ported, just a little food for thought:
I believe OSProcess is currently common between Squeak and Pharo - this porting will require that OSProcess is forked for Pharo, so the two implementations will start to diverge unless someone actively keeps them in sync. (Please don't read this as a negative judgement, it is just a statement of my understanding of the situation).
There are a number of plugin primitives that Dave added to OSProcess because FilePlugin didn't meet his requirements. Some of the recent changes made to FilePlugin reduce the need for additional primitives (it is possible to connect to an existing file or file descriptor, #atEnd works for pipes), and I think there are additional enhancements that can be made which would reduce the number even further (mark files as non-blocking) (but I haven't looked in to it properly and would want to get Dave's input first).
Cheers, Alistair
Of course we should keep the good stuff and help in maintaining OSProcess. That will probably mean that OSProcess will need dialect specific sub packages for some of its implementation. And Alistair, you did a lot of good work in this area already, we/I need you to help along. Sven
Hi Sven, On 9 May 2018 at 16:11, Sven Van Caekenberghe <sven@stfx.eu> wrote:
...
Of course we should keep the good stuff and help in maintaining OSProcess. That will probably mean that OSProcess will need dialect specific sub packages for some of its implementation.
And Alistair, you did a lot of good work in this area already, we/I need you to help along.
Thanks very much for your kind words (again). Hopefully I can continue to contribute and help improve Pharo, including all the great work you've done. Thanks again, Alistair
Hi Alistair, 2018-05-09 16:06 GMT+02:00 Alistair Grant <akgrant0710@gmail.com>:
On Wed., 9 May 2018, 13:49 Sven Van Caekenberghe, <sven@stfx.eu> wrote:
On 9 May 2018, at 13:41, Thierry Goubier <thierry.goubier@gmail.com> wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
Yes, they will have to be ported.
If it were me (but I haven't looked at the code) I would not inherit from those classes, but from a simpler Stream class, possibly even none. The stream API is actually quite small (or it should be).
Inheritance is an implementation technique, inheriting from a complex class can be hard, unless the superclass was really designed with that purpose.
All this, IMHO.
I agree that these classes will need to be ported, just a little food for thought:
I believe OSProcess is currently common between Squeak and Pharo - this porting will require that OSProcess is forked for Pharo, so the two implementations will start to diverge unless someone actively keeps them in sync. (Please don't read this as a negative judgement, it is just a statement of my understanding of the situation).
I agree with your statement. Since I need OSProcess, I will probably undertake this, even if I don't see exactly how this would be done in the context of the development process of OSProcess today. The easiest would be to have a compatibility package loaded for OSProcess on Pharo
70XXX; the hard part will be whether that XXX will be clearly known.
There are a number of plugin primitives that Dave added to OSProcess because FilePlugin didn't meet his requirements. Some of the recent changes made to FilePlugin reduce the need for additional primitives (it is possible to connect to an existing file or file descriptor, #atEnd works for pipes), and I think there are additional enhancements that can be made which would reduce the number even further (mark files as non-blocking) (but I haven't looked in to it properly and would want to get Dave's input first).
Agreed, and, yes, I've been loosely following what you were doing in this area. Anything that can improve Pharo robustness in that area will be appreciated, I had far too many lockups in the past to be happy with it. I'm a bit worried about VM changes now; I've played a bit too much with downloading my own VM from bintray for compensate for stability issues of the blessed VMs, so I'm not sure to be able to track custom OSProcess configurations depending on the presence of specific primitives in the VM... sort of like running a configure script for compiling a C library. Regards, Thierry
Cheers, Alistair
On Wed, May 09, 2018 at 01:41:47PM +0200, Thierry Goubier wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
That's right. I'm sorry I have not been keeping up with this discussion, but one thought that comes to mind - the AttachableFileStream hierarchy is something that would better be replaced by some kind of delegation. It was a hack that I did a long time ago to allow OSProcess to work without modifying the base image. But since then, we have added multibyte characters and streams, and we have different file and stream models in Squeak/Pharo/Cuis (Cuis is an important target platform from my POV). I don't have a solution in mind, my sense is that if we find a way to make AttachableFileStream go away, then that solution would very likely make it unnecessary to develop and maintain separate Squeak/Pharo/Cuis forks moving forward. Note that the only fundamental thing that AttachableFileStream does is this: AttachableFileStream class>>name: aSymbolOrString attachTo: anIOHandle writable: readWriteFlag "Create a new instance attached to anIOHandle, where anIOHandle represents an open IO channel. For write streams, this represents two Smalltalk streams which write to the same OS file or output stream, presumably with interleaved output. The purpose of this method is to permit a FileStream to be attached to an existing IOHandle, such as the IOHandle for standard input, standard output, and standard error." ^ (super basicNew name: aSymbolOrString attachTo: anIOHandle writable: readWriteFlag) initialize The method comment refers to "IOHandle" which was another old project I was doing, but IOHandle basically means the #fileID field of a file stream. If it was just AttachableFileStream that needed to be refactored out of existence, it would be easy. But I later added quite a bit a behavior to BufferedAsyncFileReadStream and I'm not sure what might be needed to make this work properly. Sven, I suspect that you are the most knowledgeable in this area, so any suggestions would be welcome. Apologies once again for not keeping up with this, but I am glad to see the discussion. Dave
Hi Dave, On 11 May 2018 at 03:58, David T. Lewis <lewis@mail.msen.com> wrote:
On Wed, May 09, 2018 at 01:41:47PM +0200, Thierry Goubier wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
That's right.
I'm sorry I have not been keeping up with this discussion, but one thought that comes to mind - the AttachableFileStream hierarchy is something that would better be replaced by some kind of delegation. It was a hack that I did a long time ago to allow OSProcess to work without modifying the base image. But since then, we have added multibyte characters and streams, and we have different file and stream models in Squeak/Pharo/Cuis (Cuis is an important target platform from my POV).
I don't have a solution in mind, my sense is that if we find a way to make AttachableFileStream go away, then that solution would very likely make it unnecessary to develop and maintain separate Squeak/Pharo/Cuis forks moving forward.
Note that the only fundamental thing that AttachableFileStream does is this:
AttachableFileStream class>>name: aSymbolOrString attachTo: anIOHandle writable: readWriteFlag "Create a new instance attached to anIOHandle, where anIOHandle represents an open IO channel. For write streams, this represents two Smalltalk streams which write to the same OS file or output stream, presumably with interleaved output. The purpose of this method is to permit a FileStream to be attached to an existing IOHandle, such as the IOHandle for standard input, standard output, and standard error."
^ (super basicNew name: aSymbolOrString attachTo: anIOHandle writable: readWriteFlag) initialize
The method comment refers to "IOHandle" which was another old project I was doing, but IOHandle basically means the #fileID field of a file stream.
If it was just AttachableFileStream that needed to be refactored out of existence, it would be easy. But I later added quite a bit a behavior to BufferedAsyncFileReadStream and I'm not sure what might be needed to make this work properly.
Sven, I suspect that you are the most knowledgeable in this area, so any suggestions would be welcome.
Are the two primitives that were added to the VM a couple of months ago (primitiveConnectToFileDescriptor & primitiveConnectToFile) enough to allow AttachableFileStream to be removed? Also, borrowing primitiveSQFileSetNonBlocking from OSProcess I was able to open a named pipe and read from it in a non-blocking way using the standard Pharo 7 file stream (BinaryFileStream). I'd like to look at getting AsyncFile working with it, but that's a long way down the ToDo list. HTH, Alistair
Hi Alistair, On Fri, May 11, 2018 at 03:09:20PM +0200, Alistair Grant wrote:
Hi Dave,
On 11 May 2018 at 03:58, David T. Lewis <lewis@mail.msen.com> wrote:
On Wed, May 09, 2018 at 01:41:47PM +0200, Thierry Goubier wrote:
2018-05-09 13:14 GMT+02:00 Sven Van Caekenberghe <sven@stfx.eu>:
You mean you made a subclass of StandardFilestream yourself ?
There are a few subclasses of StandardFileStream in OSProcess: AttachableFileStream, AsyncFileReadStream and BufferedAsyncFileReadStream. They will need to be ported.
If yes, can you describe briefly why you did so ?
As far as my analysis go, AttachableFileStream refers to and manipulates the underlying ioHandle of a unix pipe stream, AsyncFileReadStream interacts with the asynchronous io extensions of the vm to observe that io handle, and a BufferedAsyncFileReadStream add a thread-safe, buffered, blocking API for smalltalk processe on top.
That's right.
I'm sorry I have not been keeping up with this discussion, but one thought that comes to mind - the AttachableFileStream hierarchy is something that would better be replaced by some kind of delegation. It was a hack that I did a long time ago to allow OSProcess to work without modifying the base image. But since then, we have added multibyte characters and streams, and we have different file and stream models in Squeak/Pharo/Cuis (Cuis is an important target platform from my POV).
I don't have a solution in mind, my sense is that if we find a way to make AttachableFileStream go away, then that solution would very likely make it unnecessary to develop and maintain separate Squeak/Pharo/Cuis forks moving forward.
Note that the only fundamental thing that AttachableFileStream does is this:
AttachableFileStream class>>name: aSymbolOrString attachTo: anIOHandle writable: readWriteFlag "Create a new instance attached to anIOHandle, where anIOHandle represents an open IO channel. For write streams, this represents two Smalltalk streams which write to the same OS file or output stream, presumably with interleaved output. The purpose of this method is to permit a FileStream to be attached to an existing IOHandle, such as the IOHandle for standard input, standard output, and standard error."
^ (super basicNew name: aSymbolOrString attachTo: anIOHandle writable: readWriteFlag) initialize
The method comment refers to "IOHandle" which was another old project I was doing, but IOHandle basically means the #fileID field of a file stream.
If it was just AttachableFileStream that needed to be refactored out of existence, it would be easy. But I later added quite a bit a behavior to BufferedAsyncFileReadStream and I'm not sure what might be needed to make this work properly.
Sven, I suspect that you are the most knowledgeable in this area, so any suggestions would be welcome.
Are the two primitives that were added to the VM a couple of months ago (primitiveConnectToFileDescriptor & primitiveConnectToFile) enough to allow AttachableFileStream to be removed?
No, I was referring only to the actual file stream hierarchy. Long ago, I tapped into StandardFileStream with some subclasses that do useful things for OSProcess. That is clearly a hack, and I'm sure we could do better. Having said that, I have not looked at Pharo 7 yet so maybe I don't know what I'm talking about ;-)
Also, borrowing primitiveSQFileSetNonBlocking from OSProcess I was able to open a named pipe and read from it in a non-blocking way using the standard Pharo 7 file stream (BinaryFileStream). I'd like to look at getting AsyncFile working with it, but that's a long way down the ToDo list.
Good, primitiveSQFileSetNonBlocking certainly should work for that, and I'm glad to hear that it does. Dave
HTH, Alistair
Hi Dave, On 12 May 2018 at 01:28, David T. Lewis <lewis@mail.msen.com> wrote:
Hi Alistair,
On Fri, May 11, 2018 at 03:09:20PM +0200, Alistair Grant wrote:
Hi Dave,
...
Are the two primitives that were added to the VM a couple of months ago (primitiveConnectToFileDescriptor & primitiveConnectToFile) enough to allow AttachableFileStream to be removed?
No, I was referring only to the actual file stream hierarchy. Long ago, I tapped into StandardFileStream with some subclasses that do useful things for OSProcess. That is clearly a hack, and I'm sure we could do better. Having said that, I have not looked at Pharo 7 yet so maybe I don't know what I'm talking about ;-)
The two primitives aren't Pharo specific - although I don't think the primitive methods have been added to Squeak yet: primitiveConnectToFileDescriptor takes a posix file descriptor (integer) and returns a file handle (SQFile byte array). primitiveConnectToFile takes a posix FILE* pointer (byte array size FILE*) and returns a file handle (SQFile byte array). I suspect that AttachableFileStream does something else since it looks like #name:attachTo:writable: takes a SQFile byte array (I have to admit to not looking very carefully). Cheers, Alistair
participants (4)
-
Alistair Grant -
David T. Lewis -
Sven Van Caekenberghe -
Thierry Goubier