bug#20681: Build failure [MSYS2/MINGW64, OSX]

[Andreas Grünbacher]

Eli,

2015-05-31 16:29 GMT+02:00 Eli Zaretskii <eliz@gnu.org>:
>> Date: Sat, 30 May 2015 15:06:16 +0200
>> From: Andreas Grünbacher <andreas.gruenbacher@gmail.com>
>> Cc: Nick Andryshak <nandryshak@gmail.com>, 20681@debbugs.gnu.org,
>>       Paul Eggert <eggert@cs.ucla.edu>, Angelo Graziosi 
>> <angelo.graziosi@alice.it>
>>
>> > Please tell what do you want to see in confg.h, specifically.  I can
>> > force the Emacs development head to compile set-permissions.c, so
>> > perhaps Emacs's config.h will do.
>>
>> It may. I have no idea what kind of acl functionality emacs on MinGW
>> should have though.
>
> Emacs built with MinGW has the same functionality on Windows as Emacs
> does on Posix hosts: get a file's ACL as a text string, set a file's
> ACL from text string description, and copy ACL from one file to
> another.
>
> The relevant functions, acl_get_file, acl_set_file, acl_to_text,
> acl_from_text, and acl_free are implemented as part of Emacs.

So those are the functions that gnulib is seeing? If so, then emacs
could provide
acl_delete_def_file as well, and everything will be fine again.

But shouldn't Windows ACL support be added to get_permissions() and
set_permissions() proper instead of emulating Windows ACL support through
the POSIX draft ACL interface? The _to_text() and _from_text() functions
could be modified to take a struct permission_context argument; they could
be moved into gnulib or remain part of emacs.

As a side effect of that, cp in coreutils would then also do the right thing on
Windows (if someone made coreutils build there again).

>> What did it do before?
>
> The only ACL-related function that Emacs on Windows was using from
> Gnulib was acl_errno_valid.  All the rest were implemented in the
> Emacs sources.
>
> IOW, the Gnulib ACL functions were almost completely unused in the
> MinGW build.  However, they did compile, which allowed us to keep the
> Gnulib configuration very similar to what was used on Posix hosts.
> Which is why I'm not sure it is worth your while to do more than
> you've already done, unless you do want to make sure the code at least
> compiles as it did before.
>
> With that caveat, I answer below your questions, regardless.
>
>> Is the build failure new or did it already exist before the acl
>> rewrite?
>
> It is new.  Before the rewrite, the file qcopy-acl.c would compile
> with MinGW, although the function qcopy_acl was not called in the
> MinGW build, and so qcopy-acl.o was not linked into the binary.  But
> it would at least compile.
>
> After the rewrite, qcopy-acl.c is just a thin wrapper around
> get-permissions.c and set-permissions.c.  The former still compiles
> with MinGW, but the latter doesn't.  The reason is this cpp
> conditional:
>
>   #    ifndef HAVE_ACL_DELETE_DEF_FILE
>   #     error Must have acl_delete_def_file (see POSIX 1003.1e draft 17).
>   #    endif
>
> I don't understand why the code requires acl_delete_def_file where it
> previously did not: it's not like setting permissions will absolutely
> not work if there is no such function.

I see now what has changed: qcopy_acl didn't call acl_delete_def_file
before but it should have for directories. (The sibling function qset_acl
did call acl_delete_def_file even before the rewrite.)


> E.g., why not do something like the following instead:
>
>   #    ifndef HAVE_ACL_DELETE_DEF_FILE
>                     ret = -1;
>                     errno = ENOSYS;
>   #    else
>                     ret = acl_delete_def_file (name);
>
>   #    endif

Nope, the call is really needed.

> More generally, copying ACLs from one file to another does not need
> any analysis of what's in the ACLs being copied: it could simply treat
> the ACLs as an opaque object, something whose structure and semantics
> are known only to the innards of acl_get_file and acl_set_file.  So
> building qcopy_acl on top of get_permissions and set_permissions,
> which are more flexible, and do require that knowledge, is IMO
> fundamentally wrong, as it makes much harder to port this simple and
> important Gnulib function to other platforms.

I disagree. Have a look at struct permission_context; acl_get_file() and
acl_set_file() is part of the POSIX draft ACL API but other platforms
have totally different data structures and interfaces. Windows ACLs
should be implemented as another kind of ACLs, not disguised as
POSIX ACLs.

> After all, copying ACLs from one file to another is much more frequent
> and important an operation than setting ACLs from user-specified
> arguments.

Probably, yes.

> What's more, the assumption that if acl_get_file and acl_set_file are
> available, then so should be acl_delete_def_file is yet another
> obstacle, IMO gratuitous one.

No, acl_get_file, acl_set_file, and acl_delete_def_file are all part of the
POSIX ACL API, and acl_delete_def_file is needed for directories.

>> Which acl related symbols do you have in config.h?
>
> HAVE_ACL_FREE
> HAVE_ACL_FROM_TEXT
> HAVE_ACL_GET_FILE
> HAVE_ACL_SET_FILE
> HAVE_SYS_ACL_H
> USE_ACL
>
>> If it has acl_get_file and acl_set_file, where can I find
>> documentation about what those functions do on MinGW?
>
> They do what you'd expect, but support only ACL_TYPE_ACCESS.  They
> know about ACL_TYPE_DEFAULT, but always return EINVAL for it, since
> it's next to impossible (and not recommended) to have a directory on
> Windows which has no default ACLs associated with it.
>
> You can see their sources in src/w32.c in the Emacs repository.

That's just wrong. Windows ACLs can contain effective as well as inheritable
permissions; the split between types as with POSIX ACLs doesn't exist.

Thanks,
Andreas