Issue 5652 in pharo: MC incorrect announcements on class load

Status: Accepted
Owner: [hidden email]
Labels: Type-Bug

New issue 5652 by [hidden email]: MC incorrect announcements on  
class load
http://code.google.com/p/pharo/issues/detail?id=5652

1.4 #14438

When loading a class, I would expect a single ClassAdded announcement.  
Instead, we get a ClassAdded announcement, and then a  
ClassModifiedClassDefinition for the class (I think when the category is  
set), and then a ClassModifiedClassDefinition for the metaclass, but only  
if there are class-side instance variables.

1. Subscribe for class definition change announcements  
e.g. "SystemAnnouncer current on: ClassModifiedClassDefinition send:  
#classModified: to: self new."
2. Subscribe for class added announcements e.g. "SystemAnnouncer current  
on: ClassModifiedClassDefinition send: #classAdded: to: self new."
3. Load a package with MC that contains a class with a class-side instance  
variable and an instance-side instance variable

You will see that your handler from #1 gets called twice and #2 once.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixReviewNeeded

Comment #1 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

Fix in inbox:  
SLICE-Issue-5652-MC-incorrect-announcements-on-class-load-SeanDeNigris.1
* Fixed inappropriate class definition changed notifications during MC  
loading
* Fixed related bug where MC was loading classes twice

Discussion at  
http://forum.world.st/System-announcements-and-MC-td4577351.html


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Milestone-2.0

Comment #2 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #3 on issue 5652 by [hidden email]: MC incorrect announcements  
on class load
http://code.google.com/p/pharo/issues/detail?id=5652

This looks like not happening anymore. Can you check if latest 2.0 is  
having this problem?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #4 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

It seems to still be happening in latest 2.0.

1. Load the attached "Issue5652-SeanDeNigris.5.mcz"
2. Place the attached "Issue5652TestPackage-SeanDeNigris.1.mcz" in your  
package cache
3. Run Issue5652Test>>#testFixed

The test will fail.

Now evaluate "Issue5652Test fixIssue" and run the test again. It will pass.

Attachments:
        Issue5652-SeanDeNigris.5.mcz  2.3 KB
        Issue5652TestPackage-SeanDeNigris.1.mcz  955 bytes


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #5 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

Hi sean

I'm a bit picky on basicLoad because this it the main method for loading MC  
packages. So before integrating
I would to know
     - if you used your changed regularly
     - classes := additions select: [ :e | e isKindOf: MCClassDefinition ].

what happens to the non selected items?
what were the non MCClassDefinition?

Tx



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #6 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

I integrated the changed for MCClassDefinition>>load in 20122


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #7 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

> - if you used your changed regularly
Yes, I've been loading it into my personal images.

> what happens to the non selected items?
> what were the non MCClassDefinition?
It seems the only two types possible are MCClassDefinition and  
MCMethodDefinition. The classes should get loaded first, then the methods.

I started by just making that explicit with an eye toward cutting down some  
of that immense comment. After making the change, I accidentally found out  
that MC was trying to load the classes again during the method loading.




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #8 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

For your enjoyment, here are some profile results loading Seaside Base:

Original Fixed Improvement %
21219 23351 9.1
3456 3360 (-2.9)
7467 8072 7.5


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #9 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

thanks sean



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #10 on issue 5652 by marianopeck: MC incorrect announcements on  
class load
http://code.google.com/p/pharo/issues/detail?id=5652

I am lost with this issue. What do I need to review? what has been  
integrated and what hasn't?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #11 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

I integrated the part that does not touched the basicLoad method.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #12 on issue 5652 by marianopeck: MC incorrect announcements on  
class load
http://code.google.com/p/pharo/issues/detail?id=5652

Hi. I was checking the changes in #basicLoad and there is something I am  
not sure. The code was:

        additions do: [:ea | self loadClassDefinition: ea]  
displayingProgress: 'Loading classes...'.
       
       

And Sean changed to

        classes := additions select: [ :e | e isKindOf: MCClassDefinition ].
        classes do: [:ea | self loadClassDefinition: ea]  
displayingProgress: 'Loading classes...'.
       

but the #isKindOf: there is a problem.  #loadClassDefinition:  inside does  
the aDefinition isClassDefinition ifTrue:[aDefinition load]. So it looks  
*almost* the same. But isKindOf: answers tru only for MCClassDefinition and  
subclasses. It rejects the rest. However, #isClassDefinition also includes  
class traits, sin  MCClassTraitDefinition>> isClassDefinition  answers  
true.... so, in summary, with this new code, we are excluding class traits  
from loading...

sean, what do you think?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #13 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

@Mariano, great catch!! I made a new slice with just the #basicLoad  
enhancement, but using #isClassDefinition

Fix in inbox:  
SLICE-Issue-5652-MC-incorrect-announcements-on-class-load-SeanDeNigris.2

Make MCPackageLoader>>basicLoad more explicit about loading all classes  
first, and then methods. Fixed bug in v. 1 of slice that excluded traits -  
thanks Mariano.

This could be cleaned more, especially if we are sure that the additions  
collection will only ever contain method definitions and class definitions  
(i.e. where #isClassDefinition ^ true)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: MonkeyIsChecking

Comment #14 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652#c14

The Monkey is currently checking this issue. Please don't change it!


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: WorkNeeded

Comment #15 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652#c15

Monkey went bananas:
--------------------
Error while loading  
SLICE-Issue-5652-MC-incorrect-announcements-on-class-load-SeanDeNigris.2  
from http://ss3.gemstone.com/ss/PharoInbox:
        MCMergeOrLoadWarning: You are about to load new versions of the following  
packages
that have unsaved changes in the image:

   Monticello
   Monticello

If you continue, you will lose these changes:
  1: MCVersionLoader>>warnAboutLosingChangesTo:ifCancel:ifMerge:
  2: MCVersionLoader>>checkForModificationsIfCancel:ifMerge:
  3: MCVersionLoader>>loadWithNameLike:
  4: MCVersionLoader>>load
  5: GoferLoad>>execute
  6: Gofer>>execute:do:
  7: Gofer>>execute:
  8: Gofer>>load
  9: GoferResolvedReference>>load
10: [self slice load] in ChangeLoader>>loadSlice
        ...Test Results:
-------------
Passed: 5267
        CollectionsTests-Arrayed (562)
        CollectionsTests-Atomic (12)
        CollectionsTests-Sequenceable (907)
        CollectionsTests-SplitJoin (27)
        CollectionsTests-Stack (16)
        CollectionsTests-Streams (37)
        CollectionsTests-Strings (592)
        CollectionsTests-Support (12)
        CollectionsTests-Text (45)
        CollectionsTests-Unordered (1951)
        CollectionsTests-Weak (739)
        CompilerTests (179)
        SUnit-Core-Extensions (3)
        SUnit-Core-Utilities (3)
        SUnit-Tests-Core (78)
        Tests-Monticello (104)

Errors: 1
        MCDirectoryRepositoryTest>>#testAddAndLoad


----------------------------------------------------------
Loaded Source:  
SLICE-Issue-5652-MC-incorrect-announcements-on-class-load-SeanDeNigris.2  
from http://ss3.gemstone.com/ss/PharoInbox
Tested using Pharo-2.0-20172-a on CoInterpreter  
VMMaker-oscog-EstebanLorenzano.160 uuid:  
bec8cdf0-4e06-4975-8c02-e882fadf4df3 Jun 22 2012,  
StackToRegisterMappingCogit VMMaker-oscog-EstebanLorenzano.160 uuid:  
bec8cdf0-4e06-4975-8c02-e882fadf4df3 Jun 22 2012,  
https://git.gitorious.org/cogvm/blessed.git Commit:  
744bfe905c78a1a5d408680a8780367ea77e0549 Date: Fri Jun 1 15:17:41 2012  
+0200 By: Esteban Lorenzano <[hidden email]>


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #16 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

so... how are we with this issue?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Cc: [hidden email]
        Labels: -Type-Bug Type-Cleanup

Comment #17 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

My intention with the part that's left is to make the fact that classes and  
methods are loaded in separate passes more explicit in the code (in part so  
we can cut down on the huge comment). Steph had a concern because this is  
such an important method (and actually a bug was found in the fix, hee  
hee). That's where we are...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Cc: [hidden email]

Comment #18 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

forgot to cc esteban...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #19 on issue 5652 by [hidden email]: MC incorrect  
announcements on class load
http://code.google.com/p/pharo/issues/detail?id=5652

yeah... I don't know how to proceed from here :)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker