Stephane said that the biggest problem he had with Monticello was that
loading a package was not atomic.  It compiles methods one at a time,
and sometimes you need to load an entire package at once, because
loading half a package might break the compiler or some part of the
system that the package system depends on.

I decided to take a look at Monticello, because I've built other tools
that had to solve this problem.  I have attached a first attempt at a
solution.  This change makes loading a package atomic, but not
unloading it.  I will create a similar solution for unloading if
people like this.  I did not test it on any packages that require
atomic loading because I don't have any packages like that.  However,
it does run the Monticello tests, and I tested it on a couple of other
packages.  The tests only pass if you load the Make39Green.cs patch.
I don't know why, but I will figure it out some other time.

I am going to give a description of what I did.  Feel free to ignore
everything after this, but to just try out the code.  If you discover
a package that does not load properly,
please let me know.  I particularly want to know how this works on
packages that couldn't load before.

The Monticello code is fairly easy to read in spite of lacking
comments.  Class comments in particular would be useful.  There is a
large test suite that did a good job of pointing out my errors.

The key method is the basicLoad method in class MCPackageLoader.  The
original version is

basicLoad
	errorDefinitions := OrderedCollection new.
	[[additions do: [:ea | self tryToLoad: ea] displayingProgress: 'Loading...'.
	removals do: [:ea | ea unload] displayingProgress: 'Cleaning up...'.
	self shouldWarnAboutErrors ifTrue: [self warnAboutErrors].
	errorDefinitions do: [:ea | ea loadOver: (self obsoletionFor: ea)]
displayingProgress: 'Reloading...'.
	additions do: [:ea | ea postloadOver: (self obsoletionFor: ea)]
displayingProgress: 'Initializing...']
		on: InMidstOfFileinNotification
		do: [:n | n resume: true]]
			ensure: [self flushChangesFile]

The problem with this is that classes and methods are loaded by the
tryToLoad: method, one at a time.  In fact, they are also added by the
loadOver: method a little later.  I think that an errorDefinition is a
method that started out in additions but couldn't compile, so the
PackageLoader will wait until all the classes are defined and then try
to compile it again.

tryToLoad: wraps loadOver: in an exception handler and, if anything
goes wrong, puts the offending method definition in errorDefinitions.
loadOver: calls a method in ClassDescription to compile the method,
and then calls the following method to insert the method in the class.

addAndClassifySelector: selector withMethod: compiledMethod
inProtocol: category notifying: requestor
	| priorMethodOrNil |
	priorMethodOrNil _ self compiledMethodAt: selector ifAbsent: [nil].
	self addSelectorSilently: selector withMethod: compiledMethod.
	SystemChangeNotifier uniqueInstance
		doSilently: [self organization classify: selector under: category].
	priorMethodOrNil isNil
		ifTrue: [SystemChangeNotifier uniqueInstance methodAdded:
compiledMethod selector: selector inProtocol: category class: self
requestor: requestor]
		ifFalse: [SystemChangeNotifier uniqueInstance methodChangedFrom:
priorMethodOrNil to: compiledMethod selector: selector inClass: self
requestor: requestor].

Note that addSelectorSilently:withMethod: finally inserts the method
into the class.  This is the step that must be avoided.  So, I changed
this method to leave out this message, and instead returned the
compiled method.  The new method is

MCaddAndClassifySelector: selector withMethod: compiledMethod
inProtocol: category notifying: requestor
	| priorMethodOrNil |
	priorMethodOrNil _ self compiledMethodAt: selector ifAbsent: [nil].
	SystemChangeNotifier uniqueInstance
		doSilently: [self organization classify: selector under: category].
	priorMethodOrNil isNil
		ifTrue: [SystemChangeNotifier uniqueInstance methodAdded:
compiledMethod selector: selector inProtocol: category class: self
requestor: requestor]
		ifFalse: [SystemChangeNotifier uniqueInstance methodChangedFrom:
priorMethodOrNil to: compiledMethod selector: selector inClass: self
requestor: requestor].

Note that the new method tells the SystemChangeNotifier that a method
was added but doesn't actually add it until later.  I hope this
doesn't cause a problem!  An alternative would be to first insert the
methods and then to notify the SystemChangeNotifier, but to do it all
at the end of basicLoad.

I changed the method that calls it to return the compiled method.

MCcompile: text classified: category withStamp: changeStamp notifying:
requestor logSource: logSource
  "Note that I return methodAndNode, while the old version returned a selector"
	| methodAndNode |
	methodAndNode := self compile: text asString classified: category
notifying: requestor
							trailer: self defaultMethodTrailer ifFail: [^nil].
	logSource ifTrue: [
		self logMethodSource: text forMethodWithNode: methodAndNode
			inCategory: category withStamp: changeStamp notifying: requestor.
	].
	self MCaddAndClassifySelector: methodAndNode selector withMethod:
methodAndNode
		method inProtocol: category notifying: requestor.
	self instanceSide noteCompilationOf: methodAndNode selector meta:
self isClassSide.
	^ methodAndNode

So now, we can define methods in MCMethodDefinition.  loadOver is a
method that compiles the method.  It was changed to return the method
rather than installing it.  load will both compile and install the
method.

loadOver: aDefinition
	"Note that this doesn't actually load the method, it just compiles it
and returns the compiled method for loading later."
	^self actualClass
		MCcompile: source
		classified: category
		withStamp: timeStamp
		notifying: (SyntaxError new category: category)
		logSource: self actualClass acceptsLoggingOfCompilation


load
	| methodAndNode |
	methodAndNode := self loadOver: nil.
	self actualClass addSelectorSilently: selector withMethod: methodAndNode method

Here is the new version of basicLoad in MCPackageLoder.  Note that it
uses tryToLoad:in: to compile methods, but saves them in methodsToLoad
instead of installing them.  It collects some more methods from
errorDefinitions, and then installs all methods after it has processed
the errorDefinitions.  Note that the installing just uses pairsDo: on
OrderedCollection and addSelectorSilently:withMethod: on Class.  So,
it basically uses only Collection and Behavior methods.  I could make
it use even less code if that were important.

basicLoad
	| methodsToLoad |
	errorDefinitions := OrderedCollection new.
	methodsToLoad := OrderedCollection new: 10.
	[[additions do: [:ea | self tryToLoad: ea in: methodsToLoad]
displayingProgress: 'Loading...'.
	removals do: [:ea | ea unload] displayingProgress: 'Cleaning up...'.
	self shouldWarnAboutErrors ifTrue: [self warnAboutErrors].
	errorDefinitions do: [:ea | self load: ea in: methodsToLoad]
displayingProgress: 'Reloading...'.
	methodsToLoad pairsDo: [:class :eachMaN | class addSelectorSilently:
eachMaN selector withMethod: eachMaN method ].
	additions do: [:ea | ea postloadOver: (self obsoletionFor: ea)]
displayingProgress: 'Initializing...']
		on: InMidstOfFileinNotification
		do: [:n | n resume: true]]
			ensure: [self flushChangesFile]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Atomic.5.cs
Type: application/octet-stream
Size: 3912 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20061126/a9034396/Atomic.5.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Make39Green.cs
Type: application/octet-stream
Size: 20778 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20061126/a9034396/Make39Green.obj