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 <[email protected]> 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
> <[email protected]> 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 <[email protected]>
>> 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
>>> <[email protected]> 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
>>> > <[email protected]> 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
>>> >> <[email protected]> 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
>>> >>> <[email protected]> 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
