Re: [Pharo-dev] FileAttributesPlugin: request for comment and testing

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Pharo-dev] FileAttributesPlugin: request for comment and testing

Eliot Miranda-2
 
Hi Alistair,

    it looks good to me.  I have only a few quibbles.  I see a few

<var: 'winAttrs' declareC: 'WIN32_FILE_ATTRIBUTE_DATA *winAttrs'>
<var: 'accessDate' declareC: 'sqLong *accessDate'>

that are better written as var:type:

<var: 'winAttrs' type: 'WIN32_FILE_ATTRIBUTE_DATA *'>
<var: 'accessDate' type: 'sqLong *'>

and I prefer to use symbols for types, because they're usually not unique, so

<var: 'winAttrs' type: #'WIN32_FILE_ATTRIBUTE_DATA *'>
<var: 'accessDate' type: #'sqLong *'>


On Tue, Jun 12, 2018 at 8:45 AM, Alistair Grant <[hidden email]> wrote:
Hi Esteban, Guille, Marcus, Cyril and Everyone,

I'm (finally!) almost ready to merge in the FileAttributesPlugin
changes...

I have one bug to fix in the VM related to file creation, modification
and access times on Windows.

While I sort that out (and probably lose what remaining hair I have) I
was hoping that a few people could have a look at the PR on linux and /
or Mac - since it is an admittedly big one.

Note that you can't merge the changes in to an existing image as
Iceberg will attempt to use the file system in the middle of the merge
and fail. (I haven't tested this after the latest refactor, but I expect
that hasn't changed).  You'll need to use the image that has been
bootstrapped from the PR (link below).


The source code for the plugin is at: http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin

PR: https://github.com/pharo-project/pharo/pull/1529

Fogbugz:
- https://pharo.fogbugz.com/f/cases/21368/Integrate-FileAttributesPlugin
- https://pharo.fogbugz.com/f/cases/18279/

Images:

- 32 bit: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-1529/1/artifact/bootstrap-cache/Pharo7.0-32bit-af43d95.zip
- 64 bit: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-1529/1/artifact/bootstrap-cache/Pharo7.0-64bit-af43d95.zip

It requires a recent VM, from May 9 or later (the current stable VM).



What are the changes?
=====================

- Fixes FileReference>>isSymlink
  Currently it only returns true for a broken symbolic link.
- Adds the ability to retrieve symbolic link attributes.
- Extends the set of file attributes that can be retrieved to all
  attributes returned by the libc stat() and lstat() calls.
- Refactors the code to be more efficient.  In particular
  FileReference>>exists.


1. #isSymlink now works properly on Linux (and it should also work on
   MacOS and BSD).
2. The list of file attributes available from DiskDirectoryEntry now is:
    #accessTime (new)
    #changeTime (new)
    #deviceId (new)
    #gid (new)
    #inode (new)
    #isBlock (new)
    #isCharacter (new)
    #isExecutable (new)
    #isFIFO (new)
    #isRegular (new)
    #isSocket (new)
    #isSymlink (works)
    #numberOfHardLinks (new)
    #targetFile (new)
    #uid (new)

    #creationTime
    #exists
    #isDirectory
    #isFile
    #isReadable
    #isWritable
    #modificationTime
    #permissions
    #size
3. FileReference>>exists is faster than before (well, at least on my
   linux laptop).  This is useful as it is called often.
4. It is possible to retrieve symbolic link attributes, i.e. all the
   attributes listed above plus the target path.


The public interface hasn't changed.  There are a number of internal
changes to FileSystem.

As implied above, the changes are all backward compatible (except
the broken #isSymlink), although a couple deserve mention:

1. Obviously #isSymlink now answers correctly (previously it would only
  answer correctly for a broken link).
2. Requesting any of the attributes listed above (except #isSymlink)
  will return the value of the target path.  If the FileReference is to a
  broken symbolic link, it will return the attributes of the symbolic
  link (keeping existing behaviour).
3. The attributes of a symbolic link can be retrieved using
  FileReference>>symlinkEntry.

Overall, performance is slightly better than before.  Code that accesses a
single attribute through FileReference will be faster than before (it used
to retrieve all supported elements and throw the unused ones away).  Code
that needs to access multiple attributes and uses FileSystemDirectoryEntry
can access the new attributes without any additional performance cost.



Rough development timeline
==========================


April 2017: Started development

May 2017: First working version.  I've been using this in my personal
environment since May last year.

August 2017: Plugin review by Nicolas Cellier (thanks, Nicolas!)

September 2017 - January 2018 : Plugin review by Eliot Miranda (thanks
Eliot!)
This was very detailed and tidied up parameter checking, error
return codes, making the code simulator friendly, etc.

November 2017: FileAttributesPlugin added to the VM.

February 2018: Tidy up Windows specific code, fix error handling in
#creationTime.

June 2018: Refactored to place primitives in File instead of
FileAttributesPluginPrims (which matched FilePluginPrims).



Testing
=======

- As mentioned above I've been using this on a daily basis since May 2017.
  Some of the packages I have loaded include: Roassal2, Glorp,
  OSSubprocess, NeoCSV, DataFrame.
- Automated tests have been added for the new functionality.
- Additional automated tests have been added for existing functionality.
- All existing tests pass


Thanks!
Alistair




--
_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [Pharo-dev] FileAttributesPlugin: request for comment and testing

alistairgrant
 
Hi Eliot,

Thanks for taking a look.

TL;DR: I've made the changes you suggested.

On Sat, 16 Jun 2018 at 21:44, Eliot Miranda <[hidden email]> wrote:

>
>
> Hi Alistair,
>
>     it looks good to me.  I have only a few quibbles.  I see a few
>
> <var: 'winAttrs' declareC: 'WIN32_FILE_ATTRIBUTE_DATA *winAttrs'>
> <var: 'accessDate' declareC: 'sqLong *accessDate'>
>
> that are better written as var:type:

I had a few problems with the code generation and changed these while
trying to figure out what I'd done wrong.  Fixed.


> <var: 'winAttrs' type: 'WIN32_FILE_ATTRIBUTE_DATA *'>
> <var: 'accessDate' type: 'sqLong *'>
>
> and I prefer to use symbols for types, because they're usually not unique, so
>
> <var: 'winAttrs' type: #'WIN32_FILE_ATTRIBUTE_DATA *'>
> <var: 'accessDate' type: #'sqLong *'>

Hmm.  I thought we'd already changed these.  I've gone through the
entire plugin and converted them to symbols.

Thanks!
Alistair


> On Tue, Jun 12, 2018 at 8:45 AM, Alistair Grant <[hidden email]> wrote:
>>
>> Hi Esteban, Guille, Marcus, Cyril and Everyone,
>>
>> I'm (finally!) almost ready to merge in the FileAttributesPlugin
>> changes...
>>
>> I have one bug to fix in the VM related to file creation, modification
>> and access times on Windows.
>>
>> While I sort that out (and probably lose what remaining hair I have) I
>> was hoping that a few people could have a look at the PR on linux and /
>> or Mac - since it is an admittedly big one.
>>
>> Note that you can't merge the changes in to an existing image as
>> Iceberg will attempt to use the file system in the middle of the merge
>> and fail. (I haven't tested this after the latest refactor, but I expect
>> that hasn't changed).  You'll need to use the image that has been
>> bootstrapped from the PR (link below).
>>
>>
>> The source code for the plugin is at: http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin
>>
>> PR: https://github.com/pharo-project/pharo/pull/1529
>>
>> Fogbugz:
>> - https://pharo.fogbugz.com/f/cases/21368/Integrate-FileAttributesPlugin
>> - https://pharo.fogbugz.com/f/cases/18279/
>>
>> Images:
>>
>> - 32 bit: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-1529/1/artifact/bootstrap-cache/Pharo7.0-32bit-af43d95.zip
>> - 64 bit: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-1529/1/artifact/bootstrap-cache/Pharo7.0-64bit-af43d95.zip
>>
>> It requires a recent VM, from May 9 or later (the current stable VM).
>>
>>
>>
>> What are the changes?
>> =====================
>>
>> - Fixes FileReference>>isSymlink
>>   Currently it only returns true for a broken symbolic link.
>> - Adds the ability to retrieve symbolic link attributes.
>> - Extends the set of file attributes that can be retrieved to all
>>   attributes returned by the libc stat() and lstat() calls.
>> - Refactors the code to be more efficient.  In particular
>>   FileReference>>exists.
>>
>>
>> 1. #isSymlink now works properly on Linux (and it should also work on
>>    MacOS and BSD).
>> 2. The list of file attributes available from DiskDirectoryEntry now is:
>>     #accessTime (new)
>>     #changeTime (new)
>>     #deviceId (new)
>>     #gid (new)
>>     #inode (new)
>>     #isBlock (new)
>>     #isCharacter (new)
>>     #isExecutable (new)
>>     #isFIFO (new)
>>     #isRegular (new)
>>     #isSocket (new)
>>     #isSymlink (works)
>>     #numberOfHardLinks (new)
>>     #targetFile (new)
>>     #uid (new)
>>
>>     #creationTime
>>     #exists
>>     #isDirectory
>>     #isFile
>>     #isReadable
>>     #isWritable
>>     #modificationTime
>>     #permissions
>>     #size
>> 3. FileReference>>exists is faster than before (well, at least on my
>>    linux laptop).  This is useful as it is called often.
>> 4. It is possible to retrieve symbolic link attributes, i.e. all the
>>    attributes listed above plus the target path.
>>
>>
>> The public interface hasn't changed.  There are a number of internal
>> changes to FileSystem.
>>
>> As implied above, the changes are all backward compatible (except
>> the broken #isSymlink), although a couple deserve mention:
>>
>> 1. Obviously #isSymlink now answers correctly (previously it would only
>>   answer correctly for a broken link).
>> 2. Requesting any of the attributes listed above (except #isSymlink)
>>   will return the value of the target path.  If the FileReference is to a
>>   broken symbolic link, it will return the attributes of the symbolic
>>   link (keeping existing behaviour).
>> 3. The attributes of a symbolic link can be retrieved using
>>   FileReference>>symlinkEntry.
>>
>> Overall, performance is slightly better than before.  Code that accesses a
>> single attribute through FileReference will be faster than before (it used
>> to retrieve all supported elements and throw the unused ones away).  Code
>> that needs to access multiple attributes and uses FileSystemDirectoryEntry
>> can access the new attributes without any additional performance cost.
>>
>>
>>
>> Rough development timeline
>> ==========================
>>
>>
>> April 2017: Started development
>>
>> May 2017: First working version.  I've been using this in my personal
>> environment since May last year.
>>
>> August 2017: Plugin review by Nicolas Cellier (thanks, Nicolas!)
>>
>> September 2017 - January 2018 : Plugin review by Eliot Miranda (thanks
>> Eliot!)
>> This was very detailed and tidied up parameter checking, error
>> return codes, making the code simulator friendly, etc.
>>
>> November 2017: FileAttributesPlugin added to the VM.
>>
>> February 2018: Tidy up Windows specific code, fix error handling in
>> #creationTime.
>>
>> June 2018: Refactored to place primitives in File instead of
>> FileAttributesPluginPrims (which matched FilePluginPrims).
>>
>>
>>
>> Testing
>> =======
>>
>> - As mentioned above I've been using this on a daily basis since May 2017.
>>   Some of the packages I have loaded include: Roassal2, Glorp,
>>   OSSubprocess, NeoCSV, DataFrame.
>> - Automated tests have been added for the new functionality.
>> - Additional automated tests have been added for existing functionality.
>> - All existing tests pass
>>
>>
>> Thanks!
>> Alistair
>>
>
>
>
> --
> _,,,^..^,,,_
> best, Eliot