On Sat, Apr 08, 2017 at 09:41:37AM -0700, Eliot Miranda wrote:
> Hi Nicolas,
> 
> > On Apr 8, 2017, at 8:56 AM, Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com> wrote:
> > 
> > 
> > 
> > 2017-04-08 17:29 GMT+02:00 Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com>:
> >> 
> >> 
> >> 2017-04-08 17:14 GMT+02:00 Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com>:
> >>> 
> >>> 
> >>> 2017-04-08 14:09 GMT+02:00 David T. Lewis <lewis at mail.msen.com>:
> >>>> On Sat, Apr 08, 2017 at 01:59:34PM +0200, Nicolas Cellier wrote:
> >>>> > Ah, but now that I've put a
> >>>> >     (selector == #unwindTo: and: [ self name = 'Context']) ifTrue:[self
> >>>> > halt: 'debug me'].
> >>>> > I can't reproduce the problem related to removal of Context methods!!!
> >>>> >
> >>>> > Instead, I've got the 'Merging Kernel-eem.1078' window with all the
> >>>> > MethodContext->Context renaming in conflict
> >>>> > I already reported that in another thread where I told that the upgrade
> >>>> > went "smoothly" for both my 32 & 64 bits images.
> >>>> > I don't know if it's related to package cache, or .mcd, or ???
> >>>> >
> >>>> > It might also be that package ancestry is not correct du to overwriting of
> >>>> > some packages, otherwise we should not get a merge window, but well...
> >>>> >
> >>>> > Ah, but now I think I understand: it's just because I have pending changes
> >>>> > in Kernel due to the addition of halt: above (same for my images which
> >>>> > usually have some unpublished experiments).
> >>>> > We then trigger a merge rather than a load. And the merge sees conflict
> >>>> > probably because of broken ancestry (the GUID are not corresponding), but
> >>>> > once resolved manually, it processes smoothly without removing Context
> >>>> > methods...
> >>>> >
> >>>> > I can't instrument code that easily... This will have to wait for another
> >>>> > time slot :(
> >>>> >
> >>>> 
> >>>> Nicolas,
> >>>> 
> >>>> Thanks very much for working on this and for posting your results so far.
> >>>> This is not an easy problem to solve.
> >>>> 
> >>>> Dave
> >>>> 
> >>>> 
> >>> 
> >>> Hi Dave,
> >>> don't worry, real show stopper obstacles are rare, the rest of them make the game more fun ;)
> >>> So I've put the halt in MethodContext class>>removeSelector:  in a *Debug protocol so as to keep Kernel clean, and I've now got the stack at the point of removing what should not be removed.
> >>> 
> >>> It's in MCDiffyVersion load -> MCVersionLoader load -> MCPackageLoader load.
> >>> 
> >>> There's a bunch of removals, starting with:
> >>> 
> >>> an OrderedCollection(a MCMethodDefinition(MethodContext>>unwindTo:)
> >>> a MCMethodDefinition(MethodContext>>tryPrimitiveFor:receiver:args:)
> >>> a MCMethodDefinition(MethodContext>>tryNamedPrimitiveIn:for:withArgs:)
> >>> a MCMethodDefinition(MethodContext>>top)
> >>> a MCMethodDefinition(MethodContext>>terminateTo:) ...
> >>> 
> >>> So yes, this mcd is going to shoot the update process.
> >>> Why is it different from the load or merge triggered manually?
> >>> Can't we avoid all the method removals and keep only the class removal?
> >>> 
> >>> I can try this snippet in the MCPackageLoader to reject some superfluous method removals:
> >>> 
> >>> |  removedClasses |
> >>> removedClasses := (removals select: #isClassDefinition) collect: #actualClass.
> >>> removedClasses addAll: (removedClasses collect: #class).
> >>> removals := removals reject: [:e | e isMethodDefinition and: [removedClasses includes: e actualClass]]
> >>> 
> >>> Is it OK as a general change in MC?
> >>> 
> >>> Then I'm not sure how it's going to work, because at this stage, Context is still a subclass of ContextPart as I told before (though it has a different superclass), and #MethodContext is still pointing to Context, so removing either one or the other is going to lead to problems again...
> >>> 
> >>> So two more hacks might be necessary:
> >>> 
> >>> [Smalltalk globals at: #MethodContext put: Context copy] on: AttemptToWriteReadOnlyGlobal do: [:exc | exc resume: true].
> >>> 
> >>> ContextPart instVarAt: 6 put: (ContextPart subclasses select: [:e | e superclass = ContextPart]).
> >>> 
> >>> Finally, with the 2nd hack, maybe I don't need to forget the superfluous method removals.
> >>> However, it failed when I tried, so I've uploaded a Monticello-nice.667 with the hack, and triggered its download in update-eem.406.mcm
> >>> Not sure if it is really necessary...
> >>> 
> >>> Now the update is still failing with an emergency evalutator, and this time it seems to be related to the progress bar with an obsolete block still pointing to MethodContext.
> >>> 
> >>> ...snip...
> >>> UndefinedObject>>handleSignal:
> >>> Context(nil)>>handleSignal:
> >>> Context(nil)>>handleSignal:
> >>> Context(nil)>>handleSignal:
> >>> Context(nil)>>handleSignal:
> >>> 
> >>> I will retry with Bert's hack.
> >>> This update hickup is a tough one.
> >> 
> >> And Bert's hack is not enough. So another possibility would be to move #MethodContext -> Context binding in Undeclared.
> >> Undeclared shouldn't auto-clean as long as obsolete block pointing to MethodContext are still on the stack.
> >> I'm going to try this.
> > 
> > Ah yes, this time it worked for me.
> > Someone want to try and confirm?
> 
> I just updated a 64-bit image from 16987 to 17157 so I think you have it fixed.  Brilliant!
> 
> How do we capture your insights in the code?  I'll try and read the thread and the code and see that comments in the code capture things.  But not today; it's raining and we're going to the beach :-). (Muir beach and the Pelican In are fun in the rain too).
> 
> _,,,^..^,,,_ (phone)
> 

Confirming also. I updated a 64-bit image from 17086 to 17157, no problems.

Fantastic work Nicolas!

Dave