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,
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.
> 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
>> 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
>> - https://pharo.fogbugz.com/f/cases/21368/Integrate-FileAttributesPlugin
>> - https://pharo.fogbugz.com/f/cases/18279/
>> - 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
>> 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)
>> 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
>> 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
>> 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
>> June 2018: Refactored to place primitives in File instead of
>> FileAttributesPluginPrims (which matched FilePluginPrims).
>> - 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
> best, Eliot
|Free forum by Nabble||Edit this page|