Hi All, I've submitted:
https://pharo.fogbugz.com/f/cases/21431/ https://github.com/pharo-project/pharo/pull/976 The issue has additional analysis of the problem, namely that #classSide is a convenience method for retrieving the class from either an instance or the class, so isn't appropriate for chunk file out. Cheers, Alistair On 26 February 2018 at 17:43, Alistair Grant <akgrant0...@gmail.com> wrote: > Hi Everyone, > > The following demonstrates the problem (and will allow me to provide an > automated test when I submit a patch): > > > | stream definition writer diskStore nonExistent | > > definition := MCMethodDefinition > className: #DiskStore > classIsMeta: true > selector: #activeClass > category: 'current' > timeStamp: nil > source: 'activeClass > self allSubclassesDo: [:ea | > ea isActiveClass ifTrue: [^ ea]]. > ^ self'. > stream := String new writeStream. > writer := MCStWriter new > stream: stream; > yourself. > > writer writeMethodPreamble: definition. > diskStore := stream contents. > > definition := MCMethodDefinition > className: #NotALoadedClass > classIsMeta: true > selector: #reset > category: 'imagine' > timeStamp: nil > source: 'reset > > ^self doReset'. > stream := String new writeStream. > writer := MCStWriter new > stream: stream; > yourself. > > writer writeMethodPreamble: definition. > nonExistent := stream contents. > > { diskStore. nonExistent. } > > " #(' > !DiskStore class methodsFor: ''current'' stamp: ''nil''! > ' ' > !NotALoadedClass classSide methodsFor: ''imagine'' stamp: ''nil''! > ')" > > > As can be seen, the DiskStore preamble is as expected, while the > NotALoadedClass preamble uses "classSide" instead of "class". > > > Modifying one method: > > > MCStWriter>> > writeMethodPreamble: definition > self chunkContents: [:str | > stream bang. > str nextPutAll: definition className asString. > definition classIsMeta ifTrue: > [ str nextPutAll: ' class' ]. > str > nextPutAll: ' methodsFor: '; > nextPutAll: definition category asString printString; > nextPutAll: ' stamp: '; > nextPutAll: definition timeStamp asString printString > ] > > > produces the correct result, i.e. running the test code above produces: > > > " #(' > !DiskStore class methodsFor: ''current'' stamp: ''nil''! > ' ' > !NotALoadedClass class methodsFor: ''imagine'' stamp: ''nil''! > ')" > > > The NotALoadedClass has the "class" keyword as expected. > > > Cheers, > Alistair > > On 26 February 2018 at 11:14, Alistair Grant <akgrant0...@gmail.com> wrote: >> Hi Guille and Stef, >> >> Stef, thanks also for your reply. All the information I was going to >> put there I've included below, so please treat is a combined reply. >> >> >> On 26 February 2018 at 09:51, Guillermo Polito >> <guillermopol...@gmail.com> wrote: >>> Hi Alistair, >>> >>> Can you provide some more context on what are you trying to do and how? >> >> I'm working on integrating the changes I've made to FileSystem to >> properly support symbolic links and all the other attributes returned >> by libc stat(). >> >> There's a slightly dated but more detailed description of the changes >> in >> http://forum.world.st/FileSystem-file-attributes-and-isSymlink-patch-tt4956517.html >> >> The patch introduces a number of new classes to the FileSystem package: >> >> - FileAttributesPluginPrims with Unx and Windows subclasses >> - Subclasses of FileSystemDirectoryEntry for symbolic links >> - FileAttributeNotSupported >> >> >>> Generally you should not change anything in the bootstrap, just modify the >>> classes you want to modify and commit them into the repository. The >>> bootstrap will take care of it. >> >> That's true, but as mentioned this introduces new classes, which also >> should be handled automatically, but there is a bug in the Monticello >> file out code that is causing it to fail. >> >> I haven't gone through the bootstrap process in great detail, but my >> understanding is that it reads in the Pharo 7 code from the repository >> and writes out key classes to .st files to file in as part of the >> build, i.e.: >> >> ${VM70} "${COMPILER_IMAGE_NAME}.image" st ${ST_CACHE}/Multilingual.st >> ${ST_CACHE}/DeprecatedFileStream.st ${ST_CACHE}/FileSystem.st >> --no-source --quit --save >> >> The bug is only triggered because the new classes, e.g. >> FileAttributesPluginPrim, are not part of the running image. You can >> see the code here: >> >> >> MCMethodDefinition>>fullClassName >> "Using #class selector for classes for backwards compatibility" >> >> ^ self classIsMeta >> ifFalse: [self className] >> ifTrue: [ >> (self actualClass isNil or: [ self actualClass isTrait ]) >> ifFalse: [self className, ' class'] >> ifTrue: [self className, ' classSide']] >> >> >> Because FileAttributesPluginPrims isn't in the image, #actualClass >> returns nil, and the method answers "classSide" resulting in a chunk >> that looks like: >> >> >> !FileAttributesPluginPrims classSide methodsFor: 'initialize' stamp: 'nil'! >> reset >> "Reload the masks" >> >> Default := nil.! ! >> >> >> This then quietly fails to load, resulting in a DNU when the method is >> called (later on in the bootstrap process). >> >> A quick test of filing out a trait suggests this code isn't called as >> it uses "classTrait" in the chunk: >> >> >> !TestTrait classTrait methodsFor: 'testing' stamp: 'AlistairGrant >> 2/25/2018 15:37'! >> testTrait >> >> ^'testing'! ! >> >> >> I'm not familiar with the monticello code base, but #fullClassName is >> used in a number of different places, so I'm wondering whether it is >> OK to change MCStWriter>>writeMethodPreamble to always use "class". >> >> The would solve this bootstrap issue (there's one more below). >> >> >>> The only cases when you need to change bootstrap code are: >>> - your code modifies the API of the class builder, the binary >>> representation of compiled methods, requires changing some class >>> initialization or if it adds/removes some VM special object (as the ones in >>> the special objects array) >>> - you want to add a new package to pharo. In this case you need to add your >>> package to the repository and update the corresponding baseline so it is >>> loaded in the image (otherwise it will be in disk but ignored...) >> >> Or the new code is dependent on functionality introduced in the Pharo 7 VM. >> >> The Pharo 7 image is currently built using the Pharo 6 VM. Adding >> FileAttributesPlugin to the VM will require the bootstrap to use the >> Pharo 7 VM in build.sh. >> >> >> Thanks! >> Alistair >> >> >> >> >>> Guille >>> >>> On Sun, Feb 25, 2018 at 9:47 PM, Stephane Ducasse <stepharo.s...@gmail.com> >>> wrote: >>>> >>>> Now I do not understand what is this classSide here. >>>> If we have a trait apparently we do not file out a class method as >>>> class but as classSide.... Super strange. >>>> >>>> Stef >>>> >>>> On Sun, Feb 25, 2018 at 9:46 PM, Stephane Ducasse >>>> <stepharo.s...@gmail.com> wrote: >>>> > Apparently this method was like that in Pharo 60 so this is not my edit. >>>> > >>>> > fullClassName >>>> > "Using #class selector for classes for backwards compatibility" >>>> > >>>> > ^ self classIsMeta >>>> > ifFalse: [self className] >>>> > ifTrue: [ >>>> > (self actualClass isNil or: [ self actualClass isTrait ]) >>>> > ifFalse: [self className, ' class'] >>>> > ifTrue: [self className, ' classSide']] >>>> > >>>> > Now it does not mean that this is not a bug I introduced. >>>> > >>>> > >>>> > >>>> > >>>> > On Sun, Feb 25, 2018 at 9:42 PM, Stephane Ducasse >>>> > <stepharo.s...@gmail.com> wrote: >>>> >> the problem is here of course but I need my mind back (In the train to >>>> >> home). ... >>>> >> >>>> >> >>>> >> MCMethodDefinition>>fullClassName >>>> >> "Using #class selector for classes for backwards compatibility" >>>> >> >>>> >> ^ self classIsMeta >>>> >> ifFalse: [self className] >>>> >> ifTrue: [ >>>> >> (self actualClass isNil or: [ self actualClass isTrait ]) >>>> >> ifFalse: [self className, ' class'] >>>> >> ifTrue: [self className, ' classSide']] >>>> >> >>>> >> >>>> >> >>>> >> >>>> >> >>>> >> On Sun, Feb 25, 2018 at 9:41 PM, Stephane Ducasse >>>> >> <stepharo.s...@gmail.com> wrote: >>>> >>> Hi Alistair >>>> >>> >>>> >>> This is my mistake! I introduced this bug when I transformed >>>> >>> theMetaClass to classSide >>>> >>> Now I was paying super attention and I did not touch theMetaClass even >>>> >>> when class would have been far enough. >>>> >>> So do you have a scenario? So that I can try to have a look. >>>> >>> Unfortunately I was away from home and lab since a week and I may have >>>> >>> problem to allocate concentrated time. >>>> >>> But I will try. >>>> >>> Tx for reporting this! >>>> >>> >>>> >>> Stef >>>> >>> >>>> >>> >>>> >>> On Sun, Feb 25, 2018 at 3:48 PM, Alistair Grant >>>> >>> <akgrant0...@gmail.com> wrote: >>>> >>>> Hi Everyone, >>>> >>>> >>>> >>>> I'm working on integrating the file attribute primitives in to the >>>> >>>> main >>>> >>>> code during bootstrap. >>>> >>>> >>>> >>>> Class methods for FileAttributesPluginPrims are written out as: >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> !FileAttributesPluginPrims classSide methodsFor: 'initialize' stamp: >>>> >>>> 'nil'! >>>> >>>> reset >>>> >>>> "Reload the masks" >>>> >>>> >>>> >>>> Default := nil.! ! >>>> >>>> >>>> >>>> >>>> >>>> As you can see, the class name is written with "classSide" instead of >>>> >>>> "class", resulting in the method quietly being ignored during file >>>> >>>> in. >>>> >>>> >>>> >>>> This comes about in: >>>> >>>> >>>> >>>> >>>> >>>> MCStWriter>>writeMethodPreamble: definition >>>> >>>> self chunkContents: [:str | >>>> >>>> stream bang. >>>> >>>> str nextPutAll: definition fullClassName; >>>> >>>> nextPutAll: ' methodsFor: '; >>>> >>>> nextPutAll: definition category asString printString; >>>> >>>> nextPutAll: ' stamp: '; >>>> >>>> nextPutAll: definition timeStamp asString printString >>>> >>>> ] >>>> >>>> >>>> >>>> >>>> >>>> which calls: >>>> >>>> >>>> >>>> >>>> >>>> MCMethodDefinition>>fullClassName >>>> >>>> "Using #class selector for classes for backwards compatibility" >>>> >>>> >>>> >>>> ^ self classIsMeta >>>> >>>> ifFalse: [self className] >>>> >>>> ifTrue: [ >>>> >>>> (self actualClass isNil or: [ self actualClass isTrait ]) >>>> >>>> ifFalse: [self className, ' class'] >>>> >>>> ifTrue: [self className, ' classSide']] >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> Because FileAttributesPluginPrims doesn't exist in the image (the >>>> >>>> code >>>> >>>> is read in from the git repository and being written out to a .st >>>> >>>> file), >>>> >>>> #actualClass returns nil, resulting in "classSide" being used instead >>>> >>>> of >>>> >>>> "class". >>>> >>>> >>>> >>>> I've never seen "classSide" used in chunk format. Is there any time >>>> >>>> "classSide" is valid, and any suggestions on how this should be >>>> >>>> fixed? >>>> >>>> (My first thought is to just change MCStWriter>>writeMethodPreamble: >>>> >>>> to >>>> >>>> only write "class"). >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> Thanks, >>>> >>>> Alistair >>>> >>>> >>>> >>> >>> >>> >>> -- >>> >>> >>> >>> Guille Polito >>> >>> Research Engineer >>> >>> Centre de Recherche en Informatique, Signal et Automatique de Lille >>> >>> CRIStAL - UMR 9189 >>> >>> French National Center for Scientific Research - http://www.cnrs.fr >>> >>> >>> Web: http://guillep.github.io >>> >>> Phone: +33 06 52 70 66 13
