Deprecation warnings in 5.3 cause showstopper error in Seaside

> On 17.06.2020, at 01:04, tim Rowledge <[hidden email]> wrote:
>
> Here's a fun problem. Part of the 'finger pointing' even points at me for this one...
>
> After loading Seaside into that clean 5.3 image it seems impossible to get Seaside to work. All that happens is a very unhelpful page saying
> Internal Error: SystemVersion>>>  major
>
> No amount of trying adaptor restarts etc does anything different. Eventually one realises that the error handling has broken somewhere, in such a way as to prevent it even printing the error properly. This is always a fun thing to solve.
>
> Evidently somewhere a SystemVersion is being sent #major - but it does not implement it. OK, create an implementation and break on it. Dig deep into the stack and... #asMutator is sent to a Symbol, which is deprecated and that triggers the Seaside debug stack printing.
>
> So, problem 1 - the deprecation checking clearly got turned off in my Pi image and I didn't notice adequately to understand what it might change. Damn. Disabling the deprecation warnings lets Seaside 'work' - except of course is still has the problem in the error handling that will bite later.
>
> Where it goes wrong is in WAPharoWalkback>>#tempNamesAndValuesIn:do: that uses
> ` SystemVersion current major >= 8 ifTrue:[`
> ... which cannot work. Squeak hasn't implemented SystemVersion>>>  major since at least before Squeak 4.0.
>
> I also suspect that despite the WAPharoWalkback class comment claiming it is a Squeak specific class, that in fact this is a Pharo specific class. Something about the class name... I dunno? Also of course, the Squeak major version name isn't 8, it's currently 5.
>
> The good news is that simply changing the method to
> tempNamesAndValuesIn: aContext do: aTwoArgumentBlock
> SystemVersion current majorVersionNumber >= 5 ifTrue:[
> aContext tempNames doWithIndex: [ :each :index |
> aTwoArgumentBlock
> value: each
> value: (aContext namedTempAt: index) ] ]
> lets the Seaside walkback work properly.
>
> However, it seems odd that there is any guarding of the doWithIndex: clause - was there ever a time that it shouldn't have been evaluated? I don't *think* it would have been a problem pre-Closures but maybe?

> On 17.06.2020, at 01:16, Tobias Pape <[hidden email]> wrote:
>
>>
>> On 17.06.2020, at 01:04, tim Rowledge <[hidden email]> wrote:
>>
>> Here's a fun problem. Part of the 'finger pointing' even points at me for this one...
>>
>> After loading Seaside into that clean 5.3 image it seems impossible to get Seaside to work. All that happens is a very unhelpful page saying
>> Internal Error: SystemVersion>>>  major
>>
>> No amount of trying adaptor restarts etc does anything different. Eventually one realises that the error handling has broken somewhere, in such a way as to prevent it even printing the error properly. This is always a fun thing to solve.
>>
>> Evidently somewhere a SystemVersion is being sent #major - but it does not implement it. OK, create an implementation and break on it. Dig deep into the stack and... #asMutator is sent to a Symbol, which is deprecated and that triggers the Seaside debug stack printing.
>>
>> So, problem 1 - the deprecation checking clearly got turned off in my Pi image and I didn't notice adequately to understand what it might change. Damn. Disabling the deprecation warnings lets Seaside 'work' - except of course is still has the problem in the error handling that will bite later.
>>
>> Where it goes wrong is in WAPharoWalkback>>#tempNamesAndValuesIn:do: that uses
>> ` SystemVersion current major >= 8 ifTrue:[`
>> ... which cannot work. Squeak hasn't implemented SystemVersion>>>  major since at least before Squeak 4.0.
>>
>> I also suspect that despite the WAPharoWalkback class comment claiming it is a Squeak specific class, that in fact this is a Pharo specific class. Something about the class name... I dunno? Also of course, the Squeak major version name isn't 8, it's currently 5.
>>
>> The good news is that simply changing the method to
>> tempNamesAndValuesIn: aContext do: aTwoArgumentBlock
>> SystemVersion current majorVersionNumber >= 5 ifTrue:[
>> aContext tempNames doWithIndex: [ :each :index |
>> aTwoArgumentBlock
>> value: each
>> value: (aContext namedTempAt: index) ] ]
>> lets the Seaside walkback work properly.
>>
>> However, it seems odd that there is any guarding of the doWithIndex: clause - was there ever a time that it shouldn't have been evaluated? I don't *think* it would have been a problem pre-Closures but maybe?
>
> Here are the two relevant commits:
>
> https://github.com/SeasideSt/Seaside/commit/20e082861098f511fc2fea1bdb463de8560ec25f
> https://github.com/SeasideSt/Seaside/commit/3e430a7d294fdf3784abd62ec55fdfbedd20ffc8#diff-a0aa33163150c439089b33f44dec6a3f
>
> It seems pharo before 8 used a different API to access the context…
>
>
>> Also, what is the best practical option for fixing this permanently? It clearly can't work in a Pharo image so some separation will be required. Maybe add a really-Squeak specific subclass of WAPharoWalkback to the Seaside-Pharo-Development-Core package? Add a(nother) squeak related package to the configuration?
>
> We have something like these already. Seaside-Squeak-Development-Core is maybe in order.
> Or actually put that in Grease, where GemStone is already doing the right thing, more or less
> https://github.com/SeasideSt/Grease/blob/master/repository/Grease-GemStone-Core.package/GsContext.class/instance/tempNames.st
>