Fix create class in the debugger for P9

[gypsydave5]

While this is working for the original target version of Pharo (Pharo70), it's currently broken in Pharo90.

@RoelofWobben did some work on this today and found that the problem is somewhere in the ExercismPharo70 package. But where...? 🤔.

Might this also affect backward compatibility with Pharo70? And do we care?

[ghost]

Yep, the problem is described here : https://github.com/pharo-project/pharo/issues/9099 when we deleted the Pharo70 folder, the button is not greyed out but does not work properly because two commands were overwritten.

We could replace that with the orginal code back but then we loose the posibility that extensions methods are not send with the submission. Which is also not good because the mentor cannot look at all the code. See this issue : https://github.com/exercism/pharo-smalltalk/issues/184

So a very big mess.

[ghost]

I tried on P7 and there the class creation works without any problem. So I think more and more that or on P8 or P9 the code which is overwritten is changed and we need to patch that code again.,

P8 works also fine so the problem is that in P9 some code that we currently overwrite is changed.

[ghost]

so somewhere here :

Pharo 9

DoesNotUnderstand

doesNotUnderstand: aMessage

"Handle the fact that there was an attempt to send the given message to an Undeclared variable (nil), hence the receiver does not understand this message (typically #new)."

"Testing: (nil activeProcess)"

<debuggerCompleteToSender>
| exception resumeValue node |
[ 
node := self findUndeclaredVariableIn:
            thisContext outerContext sender sourceNodeExecuted ] onErrorDo: [ :ex | 
            "This is ugly, but we have a dependency with Opal compiler and 
            it should be extracted. If there is a failure during the bootstrap, this
            dependency produces an infinite loop"
     ].
node ifNil: [ ^ super doesNotUnderstand: aMessage ].

(exception := VariableNotDeclared new)
    message: aMessage;
    variableNode: node;
    receiver: self.

resumeValue := exception signal.
^ exception reachedDefaultHandler
      ifTrue: [ aMessage sentTo: self ]
      ifFalse: [ resumeValue ]

our code :

doesNotUnderstand: aMessage

"Handle the fact that there was an attempt to send the given message to an Undeclared variable (nil), hence the receiver does not understand this message (typically #new)." "Testing: (3 activeProcess)" | exception resumeValue node | (node := self findUndeclaredVariableIn: thisContext sender sourceNodeExecuted) ifNil: [ ^super doesNotUnderstand: aMessage ]. (exception := ClassNotUnderstood new) message: aMessage; variableNode: node; receiver: self. resumeValue := exception signal. ^ exception reachedDefaultHandler ifTrue: [ aMessage sentTo: self ] ifFalse: [ resumeValue ] .

[gypsydave5]

Trying really hard to understand how this relates to the #184 issue.

[ghost]

We can delete the pharo70 folder but then I think the submission of challenges which uses extension methods will break in P7 and maybe even in P9.

Like In Die where the D methods are a extension methods of I believe SmallInteger

So when a user submits this challenge, the D , D20 and D40 methods are never submitted and can be mentored

I hope this clearifies things.

[gypsydave5]

I'm not sure this is the case. Looking at the commit related to #184 (#333), the only class in ExercismPharo70 that gets updated is DoesNotUnderstandDebugAction, and that makes a change that is already in Pharo80. See the comment in the review.

https://github.com/exercism/pharo-smalltalk/pull/333/files#diff-a811eb94b800fcb97a70c0c79b567f8d96a2b811b29c4b8990470030a0c515e1

Other files in the commit seem to be more related to fixing that issue.

I suspect we can lose the package without ill effect, but hard to run tests as can't get dev environment up in P9 at the moment.

[ghost]

oke, we can try that but then I think the extensions are not submitted in P7 and still without that folder. P9 is not working properly because I want to make a new method in Object where it schould make a new class

No idea why that is not working. Maybe it has to do with that the current Pharo70 overwrites files I know the dev enviroment works in P7 or we have to wait till you or someone else solved the issue with the dev enviroment and P9.

So it looks we can solve this without or breaking P7 or P9. Bummer

[gypsydave5]

Yeah that's going to be a PITA

[ghost]

but on second thought. P7 is as far as I can see in the Pharo launcher legacy and I do not think we have to make it also work on legacy. So for me it is not a problem that things will break on P7.

[ghost]

it is maybe a easier solution to keep the Pharo70 track and put the orginal P9 code back for the two overwritten methods.

I tried for 2 days to adapt the baseline but again and again I see a error message that Pharo70 cannot be found.

[ghost]

When I put the orginal code for DoNotUnderstand in it. the right class is made but also a new command. The last one does not make any sense.

[Bajger]

Hi! Proper way of fixing that is to put updated version of method that works for P9 into P9-compatibility package and change baseline to count with Pharo version dependent package (as described here using https://github.com/pharo-open-documentation/pharo-wiki/blob/master/General/Baselines.md#loads-different-packages-depending-on-the-pharo-version)

[Bajger]

So it is vice versa: ExercismPharo70 package needs be added to baseline using e.g. by following (need to test before doing PR):

spec
                for: #(#'pharo7.x' )
                do: [
                                    spec
                        package: 'ExercismPharo70'; "Pharo override/patch methods"
                        package: 'ExercismTools' with: [ spec requires: #('ExercismPharo70') ] ]

PS: Are you aware of any methods that needs from ExercismPharo70 to be loaded for P8 or P9?

[gypsydave5]

@Bajger - haven't seen that before (or any of the Metacello stuff tbh) nice!

as far as I can see there's nothing needed for P9. Not sure about P8.

What's going to bite us is the move from OSProcess (unsupported in P8 and up) to OSSubProcess, which is in some of the tests. How would that work with the compatibility classes? Create a wrapper class to hide the concrete implementation then load different versions depending on Pharo version.

Do we need to support versions less than 9?

[ghost]

oke, learned that too and how do we say for P8 and P9 do we manually need to make a spec then. I know that for P8 the create button does what it suppose to do , so I assume that the above can be for Pharo 7 and Pharo 8

For Pharo 9 for now I think it is no problem to not using these package but we have to test if extensions methods are still submitted. If not, we have to find a solution for that maybe a Pharo 9 package.

I think we could support P8 and P9. the older versions not

[Bajger]

Hi! In general I don't think we need to break P7 compatibility by design. P7 can still work (and it is properly tested on this version of Pharo). I think it is fairly doable to have P7-P9 working versions (imagine someone still uses P7 image for submission, we don't want to break this down).

What's going to bite us is the move from OSProcess (unsupported in P8 and up) to OSSubProcess, which is in some of the tests. How would that work with the compatibility classes? Create a wrapper class to hide the concrete implementation then load different versions depending on Pharo version.

@gypsydave5 Is OSProcess (or OSSubProcess) part of standard pharo image? Can you navigate to some examples, where we refer to these packages (using its methods)?

[gypsydave5]

OSProcess is a Squeak library for spawning system processes (http://www.squeaksource.com/OSProcess). It is being used to run the configlet checking. It's being used inside the ExercismGenerator class and related tests. It's currently imported as a dependency

ExercismGenerator >> osProcess

    ^ osProcess 
        ifNotNil: [ osProcess ] 
        ifNil: [ 
            osProcess := PipeableOSProcess.
            osProcess  
             ] 

OSProcessor will not work in Pharo90 and upwards, as it depends on StandardFileStream, which has been removed now: http://forum.world.st/New-version-of-FFICHeaderExtractor-v1-0-2-and-OSubprocess-v1-3-0-td5125161.html

Could replace with LibC (https://fuhrmanator.github.io/2019/03/16/LibC-Pharo-experiments.html) or OsSubrocess, I'm not sure - busy researching this stuff myself!

[ghost]

No idea what is better. But we do not need it for testing current solutions ?

[gypsydave5]

Re: maintaining backwards compatibility. Really not sure it's worth it if we're not writing a library. I'd assume all the developers, mentors and students would be on the latest image. Is there a reason why they wouldn't be?

[gypsydave5]

No idea what is better. But we do not need it for testing current solutions ?

It's needed in the dev tooling for generating exercise files using configlet (and testing that behaviour).

So an option would be to rip out that code, and do the generation manually from the commandline.

[ghost]

could be a solution. Never made a exercise file so I do not know

@Bajger do you ever have used the configlet.

I know for v3 there is or a new version or a totally new configlet. So another issue when we are at he point of adding new challenges

[gypsydave5]

I know for v3 there is or a new version or a totally new configlet. So another issue when we are at he point of adding new challenges

Very good point

[Bajger]

Re: maintaining backwards compatibility. Really not sure it's worth it if we're not writing a library. I'd assume all the developers, mentors and students would be on the latest image. Is there a reason why they wouldn't be?

You are right: everybody can update to latest P9, they just need to pull exercises. We also need to update docs (.md) file with latest installation instructions. I would publish PR if all system tests are passing. probably this could be done using Github actions, in order to see if build is always successful when doing code changes. PS: Please ignore my previous comment (in 2nd issue related to Ring2, ExercismToolsV70 - we don't need to bother anymore if P9 is target).

[ghost]

@Bajger it looks we got it almost running. @gypsydave5 has made some changes where in the debugger I could make the methods and the classes. only one problem. The debugger wants also to make a new method.

So this is not totally solved but we are in the right direction

[gypsydave5]

@Bajger, @RoelofWobben -

We also need to update docs (.md) file with latest installation instructions.

definitely!

I would publish PR if all system tests are passing. probably this could be done using Github actions, in order to see if build is always successful when doing code changes.

Yes, getting some automation in place around this would be good. I'm not totally confident in the system tests - one of them is broken because Google changed their homepage - and I suspect some things aren't covered. But I also think that the less covered stuff is mostly in the tools for developers; students (I think) are safe.

has made some changes where in the debugger I could make the methods and the classes. only one problem. The debugger wants also to make a new method.

The only thing I'd like to fix in the PR is to improve the missing class workflow; it's broken in P9 right now, which matters little for almost everyone apart from Exercism students. Might be worth trying to patch it in our repo and revert when it's fixed in the P9 image.

PS: Please ignore my previous comment (in 2nd issue related to Ring2, ExercismToolsV70...

I learned so much from those comments, thanks!

[ghost]

As far as I know , if we get merge rights there are already some github actions that take care that the code will be on the site. I can ask that to be sure.

[gypsydave5]

Trying to have a look at the class creation stuff - rather think I'm lacking the skills to debug the debugger :rofl:

[Bajger]

The only thing I'd like to fix in the PR is to improve the missing class workflow; it's broken in P9 right now, which matters little for almost everyone apart from Exercism students. Might be worth trying to patch it in our repo and revert when it's fixed in the P9 image.

So fix would be just that Debugger reports just undeclared class, right? It seems that reporting that undefined object (nil) doesn't understand new is superfluous and Debugger as part of create action creates also dummy #new method on UndefinedObject class, which is not desired behavior. Do you know if there is reported issue on P9 describing this problem?

[ghost]

At this moment we do not know. I have asked MarcusDenker for help on this matter and he is looking if there is a bug report on this. So I wait for a respons on that in the general channel of the slack channel.

[gypsydave5]

@Bajger just to tie this off, this is the issue in P9 (P10 now I guess) describing the issue: https://github.com/pharo-project/pharo/issues/9956

[ghost]

I did not test this in P10. Im sure this is a problem in P9. And i hope one of the maintainers help us with this problem. Otherwise it will be a long way to make exercism work for Pharo amd it will not be fun anymore