[Seaside-dev] Grease on Pharo7: streams

Johan Brichau johan at inceptive.be
Wed Jul 25 17:25:05 UTC 2018 

The issue has been fixed and now Grease tests are green on Pharo 7 :)

Switching to metadataless Grease does implicate it no longer loads on Pharo 3/4.
I propose we drop those versions of Pharo moving forward such that Grease 1.3.4 is the latest one that loads on those versions. 

I’ll check what we can pickup from the dev branch as well before releasing a new version.

Johan


> On 24 Jul 2018, at 10:18, Johan Brichau <johan at inceptive.be> wrote:
> 
> So, after digging around, I have corrected my report on the broken ReadWriteStream behavior.
> 
> The problem is described here: https://pharo.fogbugz.com/f/cases/22281/ReadWriteStream-broken <https://pharo.fogbugz.com/f/cases/22281/ReadWriteStream-broken>
> 
> I’ll make a PR for Pharo and let’s see where it goes.
> 
> Johan
> 
> 
>> On 23 Jul 2018, at 21:03, Johan Brichau <johan at inceptive.be <mailto:johan at inceptive.be>> wrote:
>> 
>> Hi Paul,
>> 
>> I’ve been somewhat afloat in my mind trying to figure out what was wrong.
>> Unfortunately, I think that `(ReadWriteStream on: (String new: 4096)) contents` returning a string with null chars was due to my own changes trying to fix the tests.
>> 
>> My latest changes make fixes to Pharo to get back to the desired behavior. (https://github.com/SeasideSt/Grease/commits/pharo7 <https://github.com/SeasideSt/Grease/commits/pharo7>)
>> 
>> They actually revert 2 changes in Pharo 7 and I do not see why they were done.
>> There are also very few tests for streams in Pharo itself. I’m trying to make a PR to fix that.
>> 
>> I must correct in that (ReadWriteStream on: 'abc') contents should indeed return an empty string, as pointed out by Richard on the mailinglist in http://lists.pharo.org/pipermail/pharo-users_lists.pharo.org/2018-July/040162.html <http://lists.pharo.org/pipermail/pharo-users_lists.pharo.org/2018-July/040162.html>
>> 
>> In this respect, I discovered another change that introduced the ReadWriteStream>>on: method which, strangely, sets the readLimit position to the end of the stream. Removing this method (introduced in https://github.com/pharo-project/pharo/commit/41f3a88863f7d9b35509e6e43f7d3ac2df9801e4#diff-ba7cc7962a4a22468a18b31fe75ada04 <https://github.com/pharo-project/pharo/commit/41f3a88863f7d9b35509e6e43f7d3ac2df9801e4#diff-ba7cc7962a4a22468a18b31fe75ada04>) and reverting the change in https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04 <https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04> re-establishes the stream behavior that is tested by the stream tests in Grease.
>> 
>> I hope I’m on the right path this time ;)
>> 
>> cheers
>> Johan
>> 
>>> On 23 Jul 2018, at 20:33, Paul DeBruicker <pdebruic at gmail.com <mailto:pdebruic at gmail.com>> wrote:
>>> 
>>> 
>>> Hi Johan,
>>> 
>>> 
>>> I just downloaded Pharo 7 (on the mac, 64bit image/vm) to try this out
>>> because I was wondering if it was filling with spaces or null characters and
>>> inspecting
>>> 
>>> 
>>> (ReadWriteStream on: (String new: 4096)) contents
>>> 
>>> 
>>> gave an empty string.  Could something else cause the 4096 char string? 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> Johan Brichau-2 wrote
>>>> Hi all,
>>>> 
>>>> There have been a number of changes to the ReadWriteStream implementation
>>>> in Pharo 7 that break Grease stream tests:
>>>> https://travis-ci.org/SeasideSt/Grease/jobs/406766720 <https://travis-ci.org/SeasideSt/Grease/jobs/406766720>
>>>> <https://travis-ci.org/SeasideSt/Grease/jobs/406766720&gt <https://travis-ci.org/SeasideSt/Grease/jobs/406766720&gt>;
>>>> I think there is both an issue in Grease for Pharo and in Pharo 7 itself.
>>>> 
>>>> * The most important issue is that the following expression returns a
>>>> different result on Pharo 7 when compared with earlier versions:
>>>> 	
>>>> 	(ReadWriteStream on: (String new: 4096)) contents
>>>> 
>>>> On Pharo 6, this returns an empty string.
>>>> On Pharo 7, this returns a string with 4096 white space chars.
>>>> 
>>>> I’m inclined to say that Pharo 7 behavior is correct.
>>>> I read through the ANSI document and `contents` should return the future
>>>> sequences as well… which effectively means returning all those empty
>>>> chars.
>>>> I’m a bit in doubt why this has been different all that time… hence my
>>>> email.
>>>> 
>>>> The core of the issue is that GRPharoPlatform>>readWriteCharacterStream is
>>>> `ReadWriteStream on: (String new: 4096)`, hence a String instance with a
>>>> lot of spaces ;)
>>>> In GRGemStonePlatform, the readWriteStream is initialized on an empty
>>>> string.
>>>> In GRSqueakPlatform, the RWBinaryOrTextStream class is used on ‘ByteArray
>>>> new: 4096’.
>>>> 
>>>> Changing the implementation of GRPharoPlatform>>readWriteCharacterStream
>>>> to `ReadWriteStream on: String new` fixes most of the tests.
>>>> 
>>>> * The next issue is that `ReadWriteStream>>contents` has been changed in
>>>> https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04 <https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04>
>>>> <https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04&gt <https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04&gt>;
>>>> This means that:
>>>> 
>>>> | stream |
>>>> stream := GRPlatform current readWriteCharacterStream.
>>>> stream
>>>> 	nextPutAll: 'abc';
>>>> 	reset.
>>>> self assert: stream contents = 'abc’.
>>>> 
>>>> This is no longer true in Pharo 7.
>>>> ANSI docs says that `reset` should only reset the pointer position and
>>>> thus the future sequences on the stream should still be returned after
>>>> resetting. 
>>>> Reverting the change in the commit, also fixes that test.
>>>> It’s unclear why that change was made though, so I’ll try to talk to
>>>> someone at Pharo.
>>>> 
>>>> Would be nice to hear other’s thoughts.
>>>> 
>>>> Cheers
>>>> Johan
>>>> _______________________________________________
>>>> seaside-dev mailing list
>>> 
>>>> seaside-dev at .squeakfoundation
>>> 
>>>> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev <http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev>
>>> 
>>> 
>>> 
>>> 
>>> 
>>> --
>>> Sent from: http://forum.world.st/Seaside-Development-f1294793.html <http://forum.world.st/Seaside-Development-f1294793.html>
>>> _______________________________________________
>>> seaside-dev mailing list
>>> seaside-dev at lists.squeakfoundation.org <mailto:seaside-dev at lists.squeakfoundation.org>
>>> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev <http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/seaside-dev/attachments/20180725/a6ada0d8/attachment-0001.html>

More information about the seaside-dev mailing list