I know that code is from Martin, and I don't mean this as pointing with the finger. There are code reviews, too.

This is a case of the proverbial thin wrapper around a system function, as public API of Phobos. Amongst the large set of operating system abstractions, this one is somewhat deceiving, because it looks the same on each platform, but the parameter has a different meaning on each system. Instead of taking e.g. octal!777 it should really be a D specific enum, IMHO. It is a coincidence that both Windows and POSIX use a 32-bit integer for file attributes, but not technically sound to confuse them in a public API.

I'm posting this because concerning the parameter we have a similar situation with our file mode specifiers, which are forwarded to C library functions as is. Depending on the platform they change their meaning as well. Although there many standard flags, "don't inherit file handle in child processes" has never been standardized and cannot be retrofitted in Phobos right now, because we an enum wasn't used in the first place. </rant>

-- 
Marco

On 12/27/13 3:12 AM, Marco Leise wrote:
> I know that code is from Martin, and I don't mean this as
> pointing with the finger. There are code reviews, too.

True. Once code is accepted it is owned by the team.

> This is a case of the proverbial thin wrapper around a system
> function, as public API of Phobos. Amongst the large set of
> operating system abstractions, this one is somewhat deceiving,
> because it looks the same on each platform, but the parameter
> has a different meaning on each system. Instead of taking e.g.
> octal!777 it should really be a D specific enum, IMHO. It is a
> coincidence that both Windows and POSIX use a 32-bit integer
> for file attributes, but not technically sound to confuse them
> in a public API.

I also dislike the integer-based std.file API. The layout of the integral is inherently system-dependent so almost all code based on it is non-portable unless it uses more ad-hoc APIs (such as attrIsFile). My recent work on rdmd has revealed the std.file attribute APIs sorely wanting.

I picture two ways out of this:

1. Improve the DirEntry abstraction by e.g. allowing it to fetch attributes only etc.

2. Define a ligthly structure FileAttributes structure that encapsulates the integral and offers portable queries such as isDir, isFile etc.

Any takers?

As a general note, we are suffering from an deflation of reviewers compared to contributors. I've looked at Phobos closely again in the past couple of weeks and saw quite a few things in there that I wouldn't have approved. On the other hand, nobody can be expected to babysit Phobos 24/7 so the right response would be closer scrutiny of pull requests by the entire community. To foster that, one possibility would be to automatically post new pull requests to this group. What do you all think?


Thanks,

Andrei

Am Fri, 27 Dec 2013 09:04:39 -0800
schrieb Andrei Alexandrescu <SeeWebsiteForEmail@erdani.org>:

> I picture two ways out of this:
> 
> 1. Improve the DirEntry abstraction by e.g. allowing it to fetch attributes only etc.
> 
> 2. Define a ligthly structure FileAttributes structure that encapsulates the integral and offers portable queries such as isDir, isFile etc.
> 
> Any takers?

Wait a second, what about *setting* attributes? Some difficult ones are:

o toggling read-only (for whom? user, group, others?)
o executable flag
o hidden flag

On Windows 'executable' is implicit and based on the extension. On Posix 'hidden' is implicit for a file name beginning with a dot. We can read the hidden bit on POSIX, but we cannot toggle it for example. So we can either not expose these attributes at all, ignore them where not applicable when setting attributes or add a third state "ignore".

Or looking at it another way:
DOS attr < POSIX chmod < ACLs

How do other programming languages find a common ground?
Should Phobos add file attributes on a case by case basis?
E.g. Someone needs to toggle read-only, so we think about how
we handle that on POSIX and decide that when a file is made
writable we set the writable bits according to umask.
(http://en.wikipedia.org/wiki/Umask)
These properties can extend a DirEntry, but it is somewhat
limiting without free functions to supplement it.

> As a general note, we are suffering from an deflation of reviewers compared to contributors. I've looked at Phobos closely again in the past couple of weeks and saw quite a few things in there that I wouldn't have approved. On the other hand, nobody can be expected to babysit Phobos 24/7 so the right response would be closer scrutiny of pull requests by the entire community. To foster that, one possibility would be to automatically post new pull requests to this group. What do you all think?

Actually I'm opposed to this. It creates noise (especially if dmd/druntime changes are included that only a hand full of people understand) and GitHub already offers this functionality in the form of notifications.

"Notifications are based on the repositories you are watching.
 If you are watching a repository, you will receive notifications
 for all discussions, including:

 o Issues and their comments
 o Pull Requests and their comments
 o Comments on any commits"

It is very coarse though, so unless GitHub is open to limiting
notifications to pull requests only, it could still be useful
to post pull requests to this list.

> Thanks,
> 
> Andrei

-- 
Marco

On 12/27/2013 12:12 PM, Marco Leise wrote:
> This is a case of the proverbial thin wrapper around a system
> function, as public API of Phobos. Amongst the large set of
> operating system abstractions, this one is somewhat deceiving,
> because it looks the same on each platform, but the parameter
> has a different meaning on each system.

Yep, totally ugly but it's the counterpart to
http://dlang.org/phobos/std_file.html#.getAttributes.
So if I save these attributes in a std.zip archive I was missing the possibility to restore them.

Am Sat, 28 Dec 2013 03:50:45 +0100
schrieb Martin Nowak <code@dawg.eu>:

> On 12/27/2013 12:12 PM, Marco Leise wrote:
> > This is a case of the proverbial thin wrapper around a system function, as public API of Phobos. Amongst the large set of operating system abstractions, this one is somewhat deceiving, because it looks the same on each platform, but the parameter has a different meaning on each system.
> 
> Yep, totally ugly but it's the counterpart to
> http://dlang.org/phobos/std_file.html#.getAttributes.
> So if I save these attributes in a std.zip archive I was missing the
> possibility to restore them.

Hi, thanks for replying here. So .zip files store file
attributes as ints? Then they must also have a data member
that denotes the originating operating system, right?
Otherwise it would be impossible to correctly restore a .zip
file from one system on another. (Given relative path names
and compatible file name character sets.)
And the file attributes need to be converted between systems
as well. Otherwise it would create *very* bizarre effects when
applying POSIX chmod attributes on a Windows machine.

-- 
Marco

Am Sat, 28 Dec 2013 04:44:30 +0100
schrieb Marco Leise <Marco.Leise@gmx.de>:

> Am Sat, 28 Dec 2013 03:50:45 +0100
> schrieb Martin Nowak <code@dawg.eu>:
> 
> > On 12/27/2013 12:12 PM, Marco Leise wrote:
> > > This is a case of the proverbial thin wrapper around a system function, as public API of Phobos. Amongst the large set of operating system abstractions, this one is somewhat deceiving, because it looks the same on each platform, but the parameter has a different meaning on each system.
> > 
> > Yep, totally ugly but it's the counterpart to
> > http://dlang.org/phobos/std_file.html#.getAttributes.
> > So if I save these attributes in a std.zip archive I was missing the
> > possibility to restore them.
> 
> Hi, thanks for replying here. So .zip files store file
> attributes as ints? Then they must also have a data member
> that denotes the originating operating system, right?
> Otherwise it would be impossible to correctly restore a .zip
> file from one system on another. (Given relative path names
> and compatible file name character sets.)
> And the file attributes need to be converted between systems
> as well. Otherwise it would create *very* bizarre effects when
> applying POSIX chmod attributes on a Windows machine.

Ok, so there is a compatibility field for the file attributes
in a .zip file. So a .zip extractor has to version(Windows/Posix)
anyway to check if the attributes for a given file are
compatible with the host OS. Couldn't SetFileAttributes and
chmod be called right there on the spot?

-- 
Marco

On 12/28/2013 05:01 AM, Marco Leise wrote:
> Ok, so there is a compatibility field for the file attributes
> in a .zip file. So a .zip extractor has to version(Windows/Posix)
> anyway to check if the attributes for a given file are
> compatible with the host OS. Couldn't SetFileAttributes and
> chmod be called right there on the spot?

It does in memory extraction, you have to set the attributes after writing out the file.
https://github.com/D-Programming-Language/installer/pull/31
https://d.puremagic.com/issues/show_bug.cgi?id=11789
https://github.com/D-Programming-Language/installer/pull/33

On 12/27/2013 12:12 PM, Marco Leise wrote:
> It is a
> coincidence that both Windows and POSIX use a 32-bit integer
> for file attributes, but not technically sound to confuse them
> in a public API.

On Posix this should use mode_t instead which is 16-bit on some platforms.

Am Sat, 28 Dec 2013 05:13:57 +0100
schrieb Martin Nowak <code@dawg.eu>:

> On 12/28/2013 05:01 AM, Marco Leise wrote:
> > Ok, so there is a compatibility field for the file attributes
> > in a .zip file. So a .zip extractor has to version(Windows/Posix)
> > anyway to check if the attributes for a given file are
> > compatible with the host OS. Couldn't SetFileAttributes and
> > chmod be called right there on the spot?
> 
> It does in memory extraction, you have to set the attributes after writing out the file. https://github.com/D-Programming-Language/installer/pull/31 https://d.puremagic.com/issues/show_bug.cgi?id=11789 https://github.com/D-Programming-Language/installer/pull/33

Some backlog! So getAttributes was there already, the dmd installer script used a custom "setAttributes" to extract ZIP files, and so it was decided that setAttributes should be in Phobos, too. (in short)

Don't kill me, but I think this version of setAttributes should stay local to create_dmd_release (or std.zip if it was extended to support extraction to the file system). In the most general use case it still requires a code block like this:

  bool useFileAttr = false;
  version (Posix) {
      useFileAttr = data.isMeantForPosix;
  } else version (Windows) {
      useFileAttr = data.isMeantForWindows;
  }

  if (useFileAttr)
      std.file.setAttributes(data.fileName, data.fileAttribs);

Meaning it is (as far as I can see) only of use for external data blocks that contain OS specific file attributes in a shared 32-bit field and at that point it doesn't gain much over:

  version (Posix) {
      if (data.isMeantForPosix)
          chmod( toUTFz!(char*)(data.fileName), data.fileAttr );
  } else version (Windows) {
      if (data.isMeantForWindows)
          SetFileAttributesW( toUTFz!(wchar*)(data.fileName), data.fileAttr );
  }

For Phobos we need portable solutions. But it is also clear that std.file.setAttributes cannot be replaced to 100% by a portable solution. The question is: Does portable code _care_ about setting each possible chmod flag or Windows file attribute? Or are the use cases much more limited there, like making a file read-only or querying if it is a directory?

-- 
Marco

On Saturday, December 28, 2013 06:54:53 Marco Leise wrote:
> For Phobos we need portable solutions. But it is also clear that std.file.setAttributes cannot be replaced to 100% by a portable solution. The question is: Does portable code _care_ about setting each possible chmod flag or Windows file attribute? Or are the use cases much more limited there, like making a file read-only or querying if it is a directory?

We need to try hard to make Phobos cross-platform and portable, but some stuff just can't be, and std.file already has some functions which fall in that category (e.g. anything symlink related or dealing with file times). And having a D wrapper around that functionality can make code much cleaner, so I'm all for having a limited number of system-specific functions in std.file if that's what it takes to get the job done (and file permissions tend to fall in that category).

- Jonathan M Davis