master fails to build on FreeBSD when ACL support is on

master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Hello,

Building with commit 5af5df1 results in this error when ACL support is
turned on.

EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'  -f batch-byte-compile emacs-lisp/macroexp.el
>>Error occurred processing emacs-lisp/macroexp.el: File error (("Copying permissions to" "Invalid argument" "/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-5af5df1f7c20858dab762830e1a94994ceda425/lisp/emacs-lisp/macroexp.elc"))

The build succeeded with commit 6b8e9b7 from November 17th.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

On 01/15/2018 08:20 PM, Joseph Mingrone wrote:
> Building with commit 5af5df1 results in this error when ACL support is
> turned on.
>
> EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'  -f batch-byte-compile emacs-lisp/macroexp.el
>>> Error occurred processing emacs-lisp/macroexp.el: File error (("Copying permissions to" "Invalid argument" "/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-5af5df1f7c20858dab762830e1a94994ceda425/lisp/emacs-lisp/macroexp.elc"))
> The build succeeded with commit 6b8e9b7 from November 17th.

I don't see anything obvious that changed between those two commits, 
that would explain the problem.

What system calls are failing in the new build but succeeding in old 
one? You should be able to use truss to find that out.

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Joseph Mingrone <jrm@FreeBSD.org>
> Date: Tue, 16 Jan 2018 00:20:38 -0400
> Cc: Ashish SHUKLA <ashish@FreeBSD.org>
> 
> Building with commit 5af5df1 results in this error when ACL support is
> turned on.
> 
> EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'  -f batch-byte-compile emacs-lisp/macroexp.el
> >>Error occurred processing emacs-lisp/macroexp.el: File error (("Copying permissions to" "Invalid argument" "/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-5af5df1f7c20858dab762830e1a94994ceda425/lisp/emacs-lisp/macroexp.elc"))
> 
> The build succeeded with commit 6b8e9b7 from November 17th.

Could you try bisecting, please?

Thanks.

Re: master fails to build on FreeBSD when ACL support is on

[Ashish SHUKLA]

[-- Attachment #1.1: Type: text/plain, Size: 1097 bytes --]

On 16/01/18 5:20 AM, Joseph Mingrone wrote:
> Hello,
> 
> Building with commit 5af5df1 results in this error when ACL support is
> turned on.
> 
> EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'  -f batch-byte-compile emacs-lisp/macroexp.el
>>> Error occurred processing emacs-lisp/macroexp.el: File error (("Copying permissions to" "Invalid argument" "/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-5af5df1f7c20858dab762830e1a94994ceda425/lisp/emacs-lisp/macroexp.elc"))
> 
> The build succeeded with commit 6b8e9b7 from November 17th.
> 
> J.
> 

Hi Joseph,

I also built commit 5af5df1[1], and seems to have built fine for me.
Could you compare logs, and see what's different in your build
environment, or something ?

FTR, I'm on FreeBSD 11.1-RELEASE (amd64).

[1]
https://people.freebsd.org/~ashish/logs/emacs-devel-27.0.50.20180112_1,2.log

Thanks!
-- 
Ashish SHUKLA      | GPG: F682 CDCC 39DC 0FEA E116  20B6 C746 CFA9 E74F A4B0
freebsd.org!ashish | https://people.freebsd.org/~ashish/


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

Ashish SHUKLA <ashish@FreeBSD.org> writes:

> On 16/01/18 5:20 AM, Joseph Mingrone wrote:
>> Hello,

>> Building with commit 5af5df1 results in this error when ACL support is
>> turned on.

>> EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'  -f batch-byte-compile emacs-lisp/macroexp.el
>>>> Error occurred processing emacs-lisp/macroexp.el: File error (("Copying permissions to" "Invalid argument"
>> "/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-5af5df1f7c20858dab762830e1a94994ceda425/lisp/emacs-lisp/macroexp.elc"))

>> The build succeeded with commit 6b8e9b7 from November 17th.

>> J.


> Hi Joseph,

> I also built commit 5af5df1[1], and seems to have built fine for me.
> Could you compare logs, and see what's different in your build
> environment, or something ?

> FTR, I'm on FreeBSD 11.1-RELEASE (amd64).

> [1]
> https://people.freebsd.org/~ashish/logs/emacs-devel-27.0.50.20180112_1,2.log

> Thanks!

Hi Ashish,

This was in an 11.1-RELEASE (amd64) poudriere jail (clean environment
with the same restrictions for cluster package building).  I will follow
Paul and Eli's suggestions and report back, but it might take me another
day or so.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Mingrone <jrm@FreeBSD.org>
>> Date: Tue, 16 Jan 2018 00:20:38 -0400
>> Cc: Ashish SHUKLA <ashish@FreeBSD.org>

>> Building with commit 5af5df1 results in this error when ACL support is
>> turned on.

>> EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'  -f batch-byte-compile emacs-lisp/macroexp.el
>> >>Error occurred processing emacs-lisp/macroexp.el: File error (("Copying permissions to" "Invalid argument"
>> "/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-5af5df1f7c20858dab762830e1a94994ceda425/lisp/emacs-lisp/macroexp.elc"))

>> The build succeeded with commit 6b8e9b7 from November 17th.

> Could you try bisecting, please?

> Thanks.

The same error occurs with bf9b972 from December 2, but only when ACL is
on.  By moving back one commit to ac144dc, the build succeeds regardless
of the status of ACL.

With commit bf9b972 the value of tempfile looks something like this.
/tmp/autoload.elca4aVmU

With that commit reverted, the value of tempfile looks something like this.
/wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-694ee38/lisp/emacs-lisp/autoload.elcFmWzli

Inside the builder jail, the /tmp permissions are drwxrwxrwt.

Using the latest commit, but with the change in bf9b972 reverted, the
build succeeds.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Joseph Mingrone <jrm@FreeBSD.org>
> Cc: emacs-devel@gnu.org, Paul Eggert <eggert@cs.ucla.edu>, ashish@FreeBSD.org
> Date: Thu, 18 Jan 2018 19:40:44 -0400
> 
> The same error occurs with bf9b972 from December 2, but only when ACL is
> on.  By moving back one commit to ac144dc, the build succeeds regardless
> of the status of ACL.
> 
> With commit bf9b972 the value of tempfile looks something like this.
> /tmp/autoload.elca4aVmU
> 
> With that commit reverted, the value of tempfile looks something like this.
> /wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-694ee38/lisp/emacs-lisp/autoload.elcFmWzli
> 
> Inside the builder jail, the /tmp permissions are drwxrwxrwt.
> 
> Using the latest commit, but with the change in bf9b972 reverted, the
> build succeeds.

Thanks.  Does the patch below solve the problem?

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index acba9e2..a0ab14f 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1933,7 +1933,17 @@ byte-compile-file
 		       ;; parallel bootstrap), it does not risk getting a
 		       ;; half-finished file.  (Bug#4196)
 		       (tempfile
-                        (make-temp-file (file-name-nondirectory target-file)))
+                        ;; If target-file is relative and includes
+                        ;; leading directories, make-temp-file will
+                        ;; assume those leading directories exist
+                        ;; under temporary-file-directory, which might
+                        ;; not be true.  So strip leading directories
+                        ;; from relative file names before calling
+                        ;; make-temp-file.
+                        (if (file-name-absolute-p target-file)
+                            (make-temp-file target-file)
+                          (make-temp-file
+                           (file-name-nondirectory target-file))))
 		       (default-modes (default-file-modes))
 		       (temp-modes (logand default-modes #o600))
 		       (desired-modes (logand default-modes #o666))

Re: master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Mingrone <jrm@FreeBSD.org>
>> Cc: emacs-devel@gnu.org, Paul Eggert <eggert@cs.ucla.edu>, ashish@FreeBSD.org
>> Date: Thu, 18 Jan 2018 19:40:44 -0400

>> The same error occurs with bf9b972 from December 2, but only when ACL is
>> on.  By moving back one commit to ac144dc, the build succeeds regardless
>> of the status of ACL.

>> With commit bf9b972 the value of tempfile looks something like this.
>> /tmp/autoload.elca4aVmU

>> With that commit reverted, the value of tempfile looks something like this.
>> /wrkdirs/usr/ports/editors/emacs-devel/work-full/emacs-694ee38/lisp/emacs-lisp/autoload.elcFmWzli

>> Inside the builder jail, the /tmp permissions are drwxrwxrwt.

>> Using the latest commit, but with the change in bf9b972 reverted, the
>> build succeeds.

> Thanks.  Does the patch below solve the problem?

> diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> index acba9e2..a0ab14f 100644
> --- a/lisp/emacs-lisp/bytecomp.el
> +++ b/lisp/emacs-lisp/bytecomp.el
> @@ -1933,7 +1933,17 @@ byte-compile-file
>  		       ;; parallel bootstrap), it does not risk getting a
>  		       ;; half-finished file.  (Bug#4196)
>  		       (tempfile
> -                        (make-temp-file (file-name-nondirectory target-file)))
> +                        ;; If target-file is relative and includes
> +                        ;; leading directories, make-temp-file will
> +                        ;; assume those leading directories exist
> +                        ;; under temporary-file-directory, which might
> +                        ;; not be true.  So strip leading directories
> +                        ;; from relative file names before calling
> +                        ;; make-temp-file.
> +                        (if (file-name-absolute-p target-file)
> +                            (make-temp-file target-file)
> +                          (make-temp-file
> +                           (file-name-nondirectory target-file))))
>  		       (default-modes (default-file-modes))
>  		       (temp-modes (logand default-modes #o600))
>  		       (desired-modes (logand default-modes #o666))

It does.  Thank you.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Joseph Mingrone <jrm@FreeBSD.org>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu,  ashish@FreeBSD.org
> Date: Fri, 19 Jan 2018 10:59:16 -0400
> 
> > Thanks.  Does the patch below solve the problem?
> 
> > diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> > index acba9e2..a0ab14f 100644
> > --- a/lisp/emacs-lisp/bytecomp.el
> > +++ b/lisp/emacs-lisp/bytecomp.el
> > @@ -1933,7 +1933,17 @@ byte-compile-file
> >  		       ;; parallel bootstrap), it does not risk getting a
> >  		       ;; half-finished file.  (Bug#4196)
> >  		       (tempfile
> > -                        (make-temp-file (file-name-nondirectory target-file)))
> > +                        ;; If target-file is relative and includes
> > +                        ;; leading directories, make-temp-file will
> > +                        ;; assume those leading directories exist
> > +                        ;; under temporary-file-directory, which might
> > +                        ;; not be true.  So strip leading directories
> > +                        ;; from relative file names before calling
> > +                        ;; make-temp-file.
> > +                        (if (file-name-absolute-p target-file)
> > +                            (make-temp-file target-file)
> > +                          (make-temp-file
> > +                           (file-name-nondirectory target-file))))
> >  		       (default-modes (default-file-modes))
> >  		       (temp-modes (logand default-modes #o600))
> >  		       (desired-modes (logand default-modes #o666))
> 
> It does.  Thank you.

Thanks, I pushed it to the release branch, from which it will be
merged to master.

Re: master fails to build on FreeBSD when ACL support is on

[Stefan Monnier]

> -                        (make-temp-file (file-name-nondirectory target-file)))
> +                        ;; If target-file is relative and includes
> +                        ;; leading directories, make-temp-file will
> +                        ;; assume those leading directories exist
> +                        ;; under temporary-file-directory, which might
> +                        ;; not be true.  So strip leading directories
> +                        ;; from relative file names before calling
> +                        ;; make-temp-file.
> +                        (if (file-name-absolute-p target-file)
> +                            (make-temp-file target-file)
> +                          (make-temp-file
> +                           (file-name-nondirectory target-file))))

Hmm.. the comment here doesn't explain the (file-name-absolute-p
target-file) test, and the commit message

    * lisp/emacs-lisp/bytecomp.el (byte-compile-file): Don't create
    the temporary file under temporary-file-directory if the file
    being compiled is specified by an absolute file name.  This avoids
    problems with ACL copying from temporary-file-directory on
    FreeBSD.  For the details, see
    http://lists.gnu.org/archive/html/emacs-devel/2018-01/msg00513.html.

doesn't really explain why an absolute file name should be
treated differently.

IIUC the core of the problem has to do with different ACLs in /tmp and
the final destination of the .elc file, but it seems like this problem
should apply regardless of whether that final destination is specified
as a relative or an absolute file name, right?

So maybe rather than (file-name-absolute-p target-file) we should check
something related to ACLs, or (eq system-type 'feebsd-foo)?

Or maybe we should just never use /tmp and just go with

    (make-temp-file (expand-file-name target-file))

?


        Stefan

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 19 Jan 2018 11:38:56 -0500
> 
> > -                        (make-temp-file (file-name-nondirectory target-file)))
> > +                        ;; If target-file is relative and includes
> > +                        ;; leading directories, make-temp-file will
> > +                        ;; assume those leading directories exist
> > +                        ;; under temporary-file-directory, which might
> > +                        ;; not be true.  So strip leading directories
> > +                        ;; from relative file names before calling
> > +                        ;; make-temp-file.
> > +                        (if (file-name-absolute-p target-file)
> > +                            (make-temp-file target-file)
> > +                          (make-temp-file
> > +                           (file-name-nondirectory target-file))))
> 
> Hmm.. the comment here doesn't explain the (file-name-absolute-p
> target-file) test, and the commit message

It doesn't?

> +                        ;; If target-file is relative and includes
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^

> IIUC the core of the problem has to do with different ACLs in /tmp and
> the final destination of the .elc file, but it seems like this problem
> should apply regardless of whether that final destination is specified
> as a relative or an absolute file name, right?

Yes, the change doesn't fix the ACL problem on FreeBSD, it just allows
Emacs to be built on that system.  I don't know enough about FreeBSD
ACLs to fix the general problem, and such a solution wouldn't be
appropriate for the release branch anyway.

> So maybe rather than (file-name-absolute-p target-file) we should check
> something related to ACLs, or (eq system-type 'feebsd-foo)?

No, the change in bytecomp.el is a step in the right direction anyway,
as it's closer to what the code did before my previous change (which
uncovered the FreeBSD problem).

> Or maybe we should just never use /tmp and just go with
> 
>     (make-temp-file (expand-file-name target-file))
> 
> ?

Why is that better?

Once again, the general ACL problem on FreeBSD needs to be
investigated and solved, perhaps by declaring that system unfit for
using ACLs (if worse comes to worst).  Any other "solution" is just a
workaround, so they all just side-step the issue.

Re: master fails to build on FreeBSD when ACL support is on

[Stefan Monnier]

>> > -                        (make-temp-file (file-name-nondirectory target-file)))
>> > +                        ;; If target-file is relative and includes
>> > +                        ;; leading directories, make-temp-file will
>> > +                        ;; assume those leading directories exist
>> > +                        ;; under temporary-file-directory, which might
>> > +                        ;; not be true.  So strip leading directories
>> > +                        ;; from relative file names before calling
>> > +                        ;; make-temp-file.
>> > +                        (if (file-name-absolute-p target-file)
>> > +                            (make-temp-file target-file)
>> > +                          (make-temp-file
>> > +                           (file-name-nondirectory target-file))))
>> 
>> Hmm.. the comment here doesn't explain the (file-name-absolute-p
>> target-file) test, and the commit message
> It doesn't?

No, it doesn't explain why we create the temp file next to the target if
target is absolute and we create it in /tmp if target is relative.

>> Or maybe we should just never use /tmp and just go with
>>     (make-temp-file (expand-file-name target-file))
>> ?
> Why is that better?

Looking at the history of the code, the temp-file code started with

    (make-temp-file target-file)

which seems to me to be the result of an oversight: the original coder
probably didn't realize that such a call would either create the temp
file in the same directory as target-file or under /tmp depending on
whether target-file is relative or absolute.

Furthermore, later the code does `rename-file` which is an operation
which works much better when the old name and new new are both in the
same partition.  So I think the intention of the code was to always put
the tempfile next to the target-file.


        Stefan

Re: master fails to build on FreeBSD when ACL support is on

[Glenn Morris]

Stefan Monnier wrote:

> Looking at the history of the code, the temp-file code started with
>
>     (make-temp-file target-file)
>
> which seems to me to be the result of an oversight: the original coder
> probably didn't realize that such a call would either create the temp
> file in the same directory as target-file or under /tmp depending on
> whether target-file is relative or absolute.
>
> Furthermore, later the code does `rename-file` which is an operation
> which works much better when the old name and new new are both in the
> same partition.  So I think the intention of the code was to always put
> the tempfile next to the target-file.

You're absolutely right.
It was intentional in 0f34ae2 that the tempfile be on the same
filesystem as the target file, for the usual reasons.
I had actually been meaning to mention that 785a4a1 changed that,
but never got round to it.

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On 01/19/2018 12:53 PM, Stefan Monnier wrote:
> which seems to me to be the result of an oversight: the original coder
> probably didn't realize that such a call would either create the temp
> file in the same directory as target-file or under /tmp depending on
> whether target-file is relative or absolute.

Right you are. (The "original coder" was me; sorry about that.) Since I 
broke it, I fixed it by installing the attached into emacs-26.


[-- Attachment #2: 0001-Fix-tempfile-creation-when-byte-compiling.patch --]
[-- Type: text/x-patch, Size: 1856 bytes --]

From 1772f38eb1b51713c363d81215bfebba97c601d5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 19 Jan 2018 14:37:31 -0800
Subject: [PATCH] Fix tempfile creation when byte compiling

This improves on the recent fix for master failing to build
on FreeBSD.  Suggested by Stefan Monnier in:
https://lists.gnu.org/r/emacs-devel/2018-01/msg00600.html
* lisp/emacs-lisp/bytecomp.el (byte-compile-file):
Put tempfile next to the target file, as was the original intent.
---
 lisp/emacs-lisp/bytecomp.el | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 700a7c16b5..f6d259ba9d 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1933,17 +1933,7 @@ byte-compile-file
 		       ;; parallel bootstrap), it does not risk getting a
 		       ;; half-finished file.  (Bug#4196)
 		       (tempfile
-                        (if (file-name-absolute-p target-file)
-                            (make-temp-file target-file)
-                          ;; If target-file is relative and includes
-                          ;; leading directories, make-temp-file will
-                          ;; assume those leading directories exist
-                          ;; under temporary-file-directory, which might
-                          ;; not be true.  So strip leading directories
-                          ;; from relative file names before calling
-                          ;; make-temp-file.
-                          (make-temp-file
-                           (file-name-nondirectory target-file))))
+			(make-temp-file (expand-file-name target-file)))
 		       (default-modes (default-file-modes))
 		       (temp-modes (logand default-modes #o600))
 		       (desired-modes (logand default-modes #o666))
-- 
2.14.3

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Fri, 19 Jan 2018 15:53:33 -0500
> Cc: emacs-devel@gnu.org
> 
> >> Hmm.. the comment here doesn't explain the (file-name-absolute-p
> >> target-file) test, and the commit message
> > It doesn't?
> 
> No, it doesn't explain why we create the temp file next to the target if
> target is absolute and we create it in /tmp if target is relative.

That's how make-temp-file works, so this place is not the right one to
explain that.

Re: master fails to build on FreeBSD when ACL support is on

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

> +			(make-temp-file (expand-file-name target-file)))

What about `make-nearby-temp-file'?

Best regards, Michael.

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 19 Jan 2018 14:42:40 -0800
> 
> On 01/19/2018 12:53 PM, Stefan Monnier wrote:
> > which seems to me to be the result of an oversight: the original coder
> > probably didn't realize that such a call would either create the temp
> > file in the same directory as target-file or under /tmp depending on
> > whether target-file is relative or absolute.
> 
> Right you are. (The "original coder" was me; sorry about that.) Since I 
> broke it, I fixed it by installing the attached into emacs-26.

I've reverted it.  Please make this change on master instead, and
please don't make any non-trivial, non-documentation changes on
emacs-26 without asking first.

This particular issue has some history, and bumped into several
different use cases, each one with its unique requirements.  It's too
late now to set us back to the beginning of that history on the
release branch, because we will never be able to reliably test all
those use cases in time for the release.

More generally, someone who has access to FreeBSD and understands its
ACL implementation should look into that, because it sounds like we
have fundamental problems there with moving files from temp directory,
and those problems are just waiting to bite us in random places, like
this one.

Thanks.

Re: master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:
> More generally, someone who has access to FreeBSD and understands its
> ACL implementation should look into that, because it sounds like we
> have fundamental problems there with moving files from temp directory,
> and those problems are just waiting to bite us in random places, like
> this one.

After a big of digging, I discovered the immediate problem and it does
not have anything to do with FreeBSD's ACL implementations [1].
FreeBSD's package building tool, poudriere has an option to use tmpfs
for some of the file systems including the working directories, however
I do not believe tmpfs supports ACLs.  When I turn this option off, so
that tmpfs is not used, the build finishes successfully without any
patching.

Should ACL support be required to _build_ Emacs, even if that build will
itself support ACLs?

[1] UFS has both POSIX and NFSv4 ACLs, while ZFS only supports NFSv4 ACLs.
    https://www.freebsd.org/doc/handbook/fs-acl.html
    https://wiki.freebsd.org/NFSv4_ACLs
    https://www.freebsd.org/cgi/man.cgi?query=setfacl

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

Joseph Mingrone <jrm@FreeBSD.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>> More generally, someone who has access to FreeBSD and understands its
>> ACL implementation should look into that, because it sounds like we
>> have fundamental problems there with moving files from temp directory,
>> and those problems are just waiting to bite us in random places, like
>> this one.

> After a big of digging, I discovered the immediate problem and it does
> not have anything to do with FreeBSD's ACL implementations [1].
> FreeBSD's package building tool, poudriere has an option to use tmpfs
> for some of the file systems including the working directories, however
> I do not believe tmpfs supports ACLs.  When I turn this option off, so
> that tmpfs is not used, the build finishes successfully without any
> patching.

> Should ACL support be required to _build_ Emacs, even if that build will
> itself support ACLs?

> [1] UFS has both POSIX and NFSv4 ACLs, while ZFS only supports NFSv4 ACLs.
>     https://www.freebsd.org/doc/handbook/fs-acl.html
>     https://wiki.freebsd.org/NFSv4_ACLs
>     https://www.freebsd.org/cgi/man.cgi?query=setfacl

There is another option to have the builder do everything mounted as a
tmpfs, so neither /tmp nor the working directory supports ACLs.  In this
case, the build also finishes successfully.

To summarize, the problem only seems to occur when /tmp supports ACLs,
but $WRKDIR (location where the tarball is extracted and the build
occurs) does not.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Glenn Morris]

IMO the change in 4fd446e is obviously correct.
The current (reverted) code is wrong by inspection when target-file is
relative. In normal use, it never is, but it will be if
byte-compile-dest-file is changed to return a non-absolute value.

For example, this can be the case for automake, as we saw in

http://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00551.html

since Makefiles normally use relative filenames, and automake changes
byte-compile-dest-file-function to return "$@".

Eg I predict the following will fail on a FreeBSD system like the one in
the original message in this thread:

./src/emacs --batch \
 --eval '(setq byte-compile-dest-file-function (lambda (x) "foo.elc"))'
  -f batch-byte-compile foo.el

This simulates how automake may call Emacs to compile a file.

(It fails on RHEL 7.4 with TMPDIR=/does/not/exist. Obviously this is not
sensible, but the point is that it should not be writing to TMPDIR at all.)


I am posting this analysis for completeness, with zero expectation of it
changing anything.

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

Joseph Mingrone wrote:
> the problem only seems to occur when /tmp supports ACLs,
> but $WRKDIR (location where the tarball is extracted and the build
> occurs) does not.

The problem occurs because (rename-file A B) signals an error if A has ACLs and 
B's file system does not support ACLs. This is because rename-file calls 
(copy-file A B nil t t t) and that last "t" tells copy-file to copy ACLs.

Creating a temporary file right next to the original file should fix the problem 
for bytecomp.el, since rename-file won't be crossing file system boundaries.

The more-general question is whether rename-file should fail in this situation. 
It is a bit of an edge case. From code inspection it appears that GNU 'mv A B' 
issues a warning diagnostic but does not fail (i.e., it does not exit with 
nonzero status).

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

Glenn Morris wrote:
> I am posting this analysis for completeness, with zero expectation of it
> changing anything.

I installed the patch into master so at least master is changed, for now.

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

Michael Albinus wrote:
> What about `make-nearby-temp-file'?

When TARGET is relative and default-directory/TARGET is in a different file 
system from default-directory, I worried that (make-nearby-temp-file TARGET) 
would be worse than (make-temp-file (expand-file-name TARGET)) because it'd put 
the temp file into default-directory's file system, not into TARGET's file 
system. However, I didn't consider it all that carefully, and if you think it'd 
be better to use make-nearby-temp-file then by all means please improve the code 
(now in master, not in emacs-26).

Re: master fails to build on FreeBSD when ACL support is on

[Stefan Monnier]

> The problem occurs because (rename-file A B) signals an error if A has ACLs
> and B's file system does not support ACLs. This is because rename-file calls
> (copy-file A B nil t t t) and that last "t" tells copy-file to copy ACLs.

Talking about what happens when rename-file uses copy-file: shouldn't it
first copy-file to a temporary file near the destination and then
rename-file that temp file?  Otherwise the destination will temporarily
be in a "partially written" state, which is usually not what
`rename-file` callers want, right?


        Stefan

Re: master fails to build on FreeBSD when ACL support is on

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> Michael Albinus wrote:
>> What about `make-nearby-temp-file'?
>
> When TARGET is relative and default-directory/TARGET is in a different
> file system from default-directory, I worried that
> (make-nearby-temp-file TARGET) would be worse than (make-temp-file
> (expand-file-name TARGET)) because it'd put the temp file into
> default-directory's file system, not into TARGET's file
> system.

I have a mounted directory on "/net/ford/albinus". I get:

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/net/ford"))
  (make-nearby-temp-file "albinus/foo"))

=> "/net/ford/albinus/fooDnIYRm"

(let ((default-directory "/net/ford"))
  (make-temp-file (expand-file-name "albinus/foo")))

=> "/net/ford/albinus/foo8SyB9H"
--8<---------------cut here---------------end--------------->8---

So it is almost the same. However, it differs, if the function
`temporary-file-directory' got another implementation for the underlying
file system:

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/sftp::~/"))
  (make-nearby-temp-file "albinus"))

=> "/sftp:localhost:/tmp/albinustrjpRx"

(let ((default-directory "/sftp::~/"))
  (make-temp-file (expand-file-name "albinus")))

=> "/sftp:localhost:/home/albinus/albinusOCUmTW"
--8<---------------cut here---------------end--------------->8---

It is the responsibility of the `make-nearby-temp-file' implementation
to respect file system boundaries. I don't claim it is bug-free,
especially in the Tramp case it might not be as careful as it should
be. To be tested.

> However, I didn't consider it all that carefully, and if you
> think it'd be better to use make-nearby-temp-file then by all means
> please improve the code (now in master, not in emacs-26).

I have no chance to test it somewhere but on GNU/Linux, so I feel
uncomfortable to provide such a patch.

Best regards, Michael.

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> Cc: ashish@FreeBSD.org, monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 20 Jan 2018 19:41:32 -0800
> 
> Joseph Mingrone wrote:
> > the problem only seems to occur when /tmp supports ACLs,
> > but $WRKDIR (location where the tarball is extracted and the build
> > occurs) does not.
> 
> The problem occurs because (rename-file A B) signals an error if A has ACLs and 
> B's file system does not support ACLs. This is because rename-file calls 
> (copy-file A B nil t t t) and that last "t" tells copy-file to copy ACLs.

Why doesn't copy-file (or qcopy_acl?) use acl_errno_valid, as we do in
set-file-acl?  Is there a reason for this inconsistency?

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Joseph Mingrone <jrm@FreeBSD.org>
> Date: Sat, 20 Jan 2018 16:47:38 -0400
> Cc: Paul Eggert <eggert@cs.ucla.edu>, ashish@FreeBSD.org,
> 	monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> 
> Should ACL support be required to _build_ Emacs, even if that build will
> itself support ACLs?

There's nothing special about byte-compiling files during a build;
Emacs doesn't know it is doing that as part of building itself.

Moreover, building a release tarball doesn't require byte-compiling
any files, because all of them already come byte-compiled in the
tarball.  So this problem can only happen when building development
snapshots, and people who do that are supposed to have a working
development environment.

So I don't think we should disable ACLs while building Emacs.

Re: master fails to build on FreeBSD when ACL support is on

[Joseph Mingrone]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Mingrone <jrm@FreeBSD.org>
>> Date: Sat, 20 Jan 2018 16:47:38 -0400
>> Cc: Paul Eggert <eggert@cs.ucla.edu>, ashish@FreeBSD.org,
>> 	monnier@IRO.UMontreal.CA, emacs-devel@gnu.org

>> Should ACL support be required to _build_ Emacs, even if that build will
>> itself support ACLs?

> There's nothing special about byte-compiling files during a build;
> Emacs doesn't know it is doing that as part of building itself.

> Moreover, building a release tarball doesn't require byte-compiling
> any files, because all of them already come byte-compiled in the
> tarball.  So this problem can only happen when building development
> snapshots, and people who do that are supposed to have a working
> development environment.

> So I don't think we should disable ACLs while building Emacs.

Sounds reasonable.  Thank you all for taking the time to look into this.

Cheers,

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

Stefan Monnier wrote:
> Talking about what happens when rename-file uses copy-file: shouldn't it
> first copy-file to a temporary file near the destination and then
> rename-file that temp file?  Otherwise the destination will temporarily
> be in a "partially written" state, which is usually not what
> `rename-file` callers want, right?
I suspect rename-file is trying to mimic 'mv' here. With GNU 'mv', the 
destination can temporarily be in a partially-written state when file system 
boundaries are being crossed. This behavior is allowed by POSIX. Although the 
behavior you describe does have advantages, it is not allowed by POSIX for 'mv' 
and it can exhaust the destination file system even when there's room for the 
final result.

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

Eli Zaretskii wrote:
> Why doesn't copy-file (or qcopy_acl?) use acl_errno_valid, as we do in
> set-file-acl?  Is there a reason for this inconsistency?

set-file-acl is documented to return t on success, nil on ordinary failure, and 
(this part is undocumented) it signals an error on a serious failure. 
acl_errno_valid is used to decide whether the error is serious.

copy-file simply signals an error on failure; its return value is undocumented. 
So it doesn't have the three possibilities that set-file-acl has.

Perhaps copy-file's API should be changed to have those three possibilities, but 
that'd be a fairly big change to its API.

Re: master fails to build on FreeBSD when ACL support is on

[Stefan Monnier]

> I suspect rename-file is trying to mimic 'mv' here. With GNU 'mv', the
> destination can temporarily be in a partially-written state when file system
> boundaries are being crossed. This behavior is allowed by POSIX. Although
> the behavior you describe does have advantages, it is not allowed by POSIX
> for 'mv' and it can exhaust the destination file system even when there's
> room for the final result.

Hmm... regarding the problem of transient space usage, we could simply
remove the target, then copy to the temp file, then move to target.

We can choose one of the downsides:
- a transient state where the target is missing
- a transient state where the target is partially written
- a transient state where we use more disk space

Not sure why POSIX chose the middle downside.  I personally much prefer
the third one.


        Stefan

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> Cc: jrm@ftfl.ca, ashish@FreeBSD.org, monnier@IRO.UMontreal.CA,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 21 Jan 2018 22:52:02 -0800
> 
> Eli Zaretskii wrote:
> > Why doesn't copy-file (or qcopy_acl?) use acl_errno_valid, as we do in
> > set-file-acl?  Is there a reason for this inconsistency?
> 
> set-file-acl is documented to return t on success, nil on ordinary failure, and 
> (this part is undocumented) it signals an error on a serious failure. 
> acl_errno_valid is used to decide whether the error is serious.
> 
> copy-file simply signals an error on failure; its return value is undocumented. 
> So it doesn't have the three possibilities that set-file-acl has.

I didn't mean to change the API, I meant to ask why doesn't copy-file
use acl_errno_valid, and if that says the error just means ACLs aren't
supported in this case, silently gives up on copying ACLs?  The return
value doesn't need to change.  We use set-file-acl when we need a more
fancy copying, which we do in Lisp, so why not in the primitive?

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

On 01/22/2018 07:52 AM, Eli Zaretskii wrote:
> I didn't mean to change the API, I meant to ask why doesn't copy-file
> use acl_errno_valid, and if that says the error just means ACLs aren't
> supported in this case, silently gives up on copying ACLs?  The return
> value doesn't need to change.  We use set-file-acl when we need a more
> fancy copying, which we do in Lisp, so why not in the primitive?

You asked why the two functions are inconsistent, and my answer was that 
the inconsistency springs from the fact that they have different APIs. 
The two functions would continue to be inconsistent even if we were to 
change the behavior in the way that you suggest, since set-file-acl 
would inform callers whether ACL setting failed (by returning nil 
instead of t), whereas copy-file would not. If consistency is the goal 
then we need to change their APIs somehow.

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> Cc: jrm@ftfl.ca, ashish@FreeBSD.org, monnier@IRO.UMontreal.CA,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 22 Jan 2018 09:02:47 -0800
> 
> You asked why the two functions are inconsistent, and my answer was that 
> the inconsistency springs from the fact that they have different APIs. 
> The two functions would continue to be inconsistent even if we were to 
> change the behavior in the way that you suggest, since set-file-acl 
> would inform callers whether ACL setting failed (by returning nil 
> instead of t), whereas copy-file would not. If consistency is the goal 
> then we need to change their APIs somehow.

Why do we need to inform the callers that ACL setting failed, when we
detect that ACLs are not supported by the underlying filesystem?  What
would be the practical usefulness of such an indication?

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

On 01/22/2018 09:41 AM, Eli Zaretskii wrote:
> Why do we need to inform the callers that ACL setting failed

Because they invoked copy-file with a non-nil preserve-permissions flag, 
and copy-file cannot preserve the permissions as requested. It's the 
same reason copy-file signals an error when invoked with a non-nil 
keep-time flag and when it cannot keep the time on the output file. If 
copy-file did not report an error, users would be lulled into thinking 
that the destination has the same permissions as the source when 
copy-file succeeds, even though that's not the case.

There is some precedent for ignoring failure, as copy-file does ignore 
chown failure when the preserve-uid-gid flag is used. However, this 
behavior is documented as a specific exception to the general rule that 
copy-file signals failures.

One way to move forward would be to change copy-file to have a three-way 
result, as set-file-acl does. That is, it could return t if successful, 
nil if mildly unsuccessful, and signal an error if severely 
unsuccessful. Failure to preserve UID and GID would be considered mild. 
Perhaps failure to preserve permissions could be considered mild, too, 
since it's no more security-relevant than UID failure is.

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> Cc: jrm@ftfl.ca, ashish@FreeBSD.org, monnier@IRO.UMontreal.CA,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 22 Jan 2018 10:50:59 -0800
> 
> On 01/22/2018 09:41 AM, Eli Zaretskii wrote:
> > Why do we need to inform the callers that ACL setting failed
> 
> Because they invoked copy-file with a non-nil preserve-permissions flag, 
> and copy-file cannot preserve the permissions as requested. It's the 
> same reason copy-file signals an error when invoked with a non-nil 
> keep-time flag and when it cannot keep the time on the output file. If 
> copy-file did not report an error, users would be lulled into thinking 
> that the destination has the same permissions as the source when 
> copy-file succeeds, even though that's not the case.

Failure to keep time means a real problem that shouldn't just happen
because of some feature of the filesystem that isn't supported.  Any
reasonable filesystem supports time-stamping files.  By contrast, ACLs
are not universally supported, and giving up on copying them doesn't
mean the file is not protected.  In fact, the user didn't even ask us
to preserve ACLs, we just do it because it's a useful thing when it
works.  When it doesn't, it isn't a catastrophe, because the "normal"
protection is still there.

Anyway, I see that we disagree, so I will just stop trying.

> One way to move forward would be to change copy-file to have a three-way 
> result, as set-file-acl does. That is, it could return t if successful, 
> nil if mildly unsuccessful, and signal an error if severely 
> unsuccessful.

The distinction between return values is not relevant to interactive
invocation.

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

On 01/22/2018 12:32 PM, Eli Zaretskii wrote:
>> One way to move forward would be to change copy-file to have a three-way
>> result, as set-file-acl does. That is, it could return t if successful,
>> nil if mildly unsuccessful, and signal an error if severely
>> unsuccessful.
> The distinction between return values is not relevant to interactive
> invocation.

Sure, but this topic came up for a noninteractive invocation: the place 
where rename-file calls copy-file. If rename-file had a way of telling 
copy-file "Hey! use your best effort to copy permissions but don't 
signal an error if you can't", then rename-file could suppress the 
signal for failure to copy ACLs without suppressing the signal for other 
failures. (This may not be a good idea, though; please see below.)

> ACLs are not universally supported, and giving up on copying them doesn't
> mean the file is not protected.
I'm afraid that it does mean that, at least for GNU/Linux ACLs, as they 
can prevent you from accessing a file that you could otherwise access. 
In other words, if you copy a file but not its ACLs, a user may be able 
to access the copy even though they cannot access the original. This is 
an argument against silently continuing when ACLs cannot be copied.

Re: master fails to build on FreeBSD when ACL support is on

[Glenn Morris]

Glenn Morris wrote:

> IMO the change in 4fd446e is obviously correct.
> The current (reverted) code is wrong by inspection when target-file is
> relative. In normal use, it never is, but it will be if
> byte-compile-dest-file is changed to return a non-absolute value.
>
> For example, this can be the case for automake, as we saw in
>
> http://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00551.html
>
> since Makefiles normally use relative filenames, and automake changes
> byte-compile-dest-file-function to return "$@".
>
> Eg I predict the following will fail on a FreeBSD system like the one in
> the original message in this thread:
>
> ./src/emacs --batch \
>  --eval '(setq byte-compile-dest-file-function (lambda (x) "foo.elc"))'
>   -f batch-byte-compile foo.el
>
> This simulates how automake may call Emacs to compile a file.
>
> (It fails on RHEL 7.4 with TMPDIR=/does/not/exist. Obviously this is not
> sensible, but the point is that it should not be writing to TMPDIR at all.)


Here is an instance of the failure that I predicted:

http://lists.nongnu.org/r/bbdb-user/2018-11/msg00003.html

Please consider backporting 64c8467 (the master version of the reverted
4fd446e from emacs-26) to the emacs-26 branch.

Re: master fails to build on FreeBSD when ACL support is on

[Paul Eggert]

On 11/14/18 3:12 PM, Glenn Morris wrote:
> Here is an instance of the failure that I predicted:
>
> http://lists.nongnu.org/r/bbdb-user/2018-11/msg00003.html
>
> Please consider backporting 64c8467 (the master version of the reverted
> 4fd446e from emacs-26) to the emacs-26 branch.

Sounds good to me. As I understand from 
<https://lists.gnu.org/archive/html/emacs-devel/2018-01/msg00607.html>, 
the fix was reverted from emacs-26 because we were too close to a 
release then. If we're not too close to an emacs-26 release now, then 
now would be a good time to reinstall the fix.

Re: master fails to build on FreeBSD when ACL support is on

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 14 Nov 2018 17:23:50 -0800
> Cc: emacs-devel@gnu.org
> 
> On 11/14/18 3:12 PM, Glenn Morris wrote:
> > Here is an instance of the failure that I predicted:
> >
> > http://lists.nongnu.org/r/bbdb-user/2018-11/msg00003.html
> >
> > Please consider backporting 64c8467 (the master version of the reverted
> > 4fd446e from emacs-26) to the emacs-26 branch.
> 
> Sounds good to me. As I understand from 
> <https://lists.gnu.org/archive/html/emacs-devel/2018-01/msg00607.html>, 
> the fix was reverted from emacs-26 because we were too close to a 
> release then. If we're not too close to an emacs-26 release now, then 
> now would be a good time to reinstall the fix.

We _are_ close to releasing Emacs 26.2; a pretest should be announced
soon.

However, we are also wiser by almost a year, and both versions were
hopefully tested well enough, each one on its branch.  So I went ahead
and cherry-picked the change from master.

Thanks.