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]