[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGde0a22471157: Remove umask tests (authored by aganea).

Changed prior to commit:
  https://reviews.llvm.org/D70854?vs=233402&id=237470#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854

Files:
  clang/test/Misc/permissions.cpp
  llvm/test/Other/umask.ll


Index: llvm/test/Other/umask.ll
===
--- llvm/test/Other/umask.ll
+++ /dev/null
@@ -1,14 +0,0 @@
-; REQUIRES: shell
-; XFAIL: windows-gnu
-
-; RUN: umask 000
-; RUN: rm -f %t.000
-; RUN: llvm-as %s -o %t.000
-; RUN: ls -l %t.000 | FileCheck --check-prefix=CHECK000 %s
-; CHECK000: rw-rw-rw
-
-; RUN: umask 002
-; RUN: rm -f %t.002
-; RUN: llvm-as %s -o %t.002
-; RUN: ls -l %t.002 | FileCheck --check-prefix=CHECK002 %s
-; CHECK002: rw-rw-r-
Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ /dev/null
@@ -1,11 +0,0 @@
-// REQUIRES: shell
-
-// RUN: umask 000
-// RUN: %clang_cc1 -emit-llvm-bc %s -o %t
-// RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s
-// CHECK000: rw-rw-rw-
-
-// RUN: umask 002
-// RUN: %clang_cc1 -emit-llvm-bc %s -o %t
-// RUN: ls -l %t | FileCheck --check-prefix=CHECK002 %s
-// CHECK002: rw-rw-r--


Index: llvm/test/Other/umask.ll
===
--- llvm/test/Other/umask.ll
+++ /dev/null
@@ -1,14 +0,0 @@
-; REQUIRES: shell
-; XFAIL: windows-gnu
-
-; RUN: umask 000
-; RUN: rm -f %t.000
-; RUN: llvm-as %s -o %t.000
-; RUN: ls -l %t.000 | FileCheck --check-prefix=CHECK000 %s
-; CHECK000: rw-rw-rw
-
-; RUN: umask 002
-; RUN: rm -f %t.002
-; RUN: llvm-as %s -o %t.002
-; RUN: ls -l %t.002 | FileCheck --check-prefix=CHECK002 %s
-; CHECK002: rw-rw-r-
Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ /dev/null
@@ -1,11 +0,0 @@
-// REQUIRES: shell
-
-// RUN: umask 000
-// RUN: %clang_cc1 -emit-llvm-bc %s -o %t
-// RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s
-// CHECK000: rw-rw-rw-
-
-// RUN: umask 002
-// RUN: %clang_cc1 -emit-llvm-bc %s -o %t
-// RUN: ls -l %t | FileCheck --check-prefix=CHECK002 %s
-// CHECK002: rw-rw-r--
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 233402.
aganea added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Ensure the access-rights commands (setfacl and chmod) won't fail the tests if 
the user doesn't have the appropriate rights to change permissions.
Added `llvm/test/Other/umask.ll` because it was also failing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854

Files:
  clang/test/Misc/permissions.cpp
  llvm/test/Other/umask.ll


Index: llvm/test/Other/umask.ll
===
--- llvm/test/Other/umask.ll
+++ llvm/test/Other/umask.ll
@@ -1,6 +1,9 @@
 ; REQUIRES: shell
 ; XFAIL: windows-gnu
 
+; RUN: setfacl -bn %T || true
+; RUN: chmod -R o+rx %T || true
+
 ; RUN: umask 000
 ; RUN: rm -f %t.000
 ; RUN: llvm-as %s -o %t.000
Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ clang/test/Misc/permissions.cpp
@@ -1,5 +1,8 @@
 // REQUIRES: shell
 
+// RUN: setfacl -bn %T || true
+// RUN: chmod -R o+rx %T || true
+
 // RUN: umask 000
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s


Index: llvm/test/Other/umask.ll
===
--- llvm/test/Other/umask.ll
+++ llvm/test/Other/umask.ll
@@ -1,6 +1,9 @@
 ; REQUIRES: shell
 ; XFAIL: windows-gnu
 
+; RUN: setfacl -bn %T || true
+; RUN: chmod -R o+rx %T || true
+
 ; RUN: umask 000
 ; RUN: rm -f %t.000
 ; RUN: llvm-as %s -o %t.000
Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ clang/test/Misc/permissions.cpp
@@ -1,5 +1,8 @@
 // REQUIRES: shell
 
+// RUN: setfacl -bn %T || true
+// RUN: chmod -R o+rx %T || true
+
 // RUN: umask 000
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I see there is no setfacl on Mac. Personally, I feel like this test doesn't 
have much value. Rafael added it long ago when we changed how we opened files 
in some way that I guess affected umask. I'm not sure we really need this 
regression test given how unportable it seems to be.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Yes this was added in rG18627115f4d2db5dc73207e0b5312f52536be7dd 
 and 
rGe08b59f81d950bd5c8b8528fcb3ac4230c7b736c 
. It looks 
like it was only there to validate that specific refactoring.
Should I remove these two files, or simply commit the previous diff? (ie. 
-rw-rw)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think we should remove it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: dblaikie, rnk.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
aganea retitled this revision from "[Clang] Do not always assume others 
permissions are set" to "[Clang] In tests, do not always assume others 
permissions are set".

On my Linux Ubuntu box, permissions for "others" are always unset/empty, thus 
`clang/test/Misc/permissions.cpp` used to always fail.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70854

Files:
  clang/test/Misc/permissions.cpp


Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ clang/test/Misc/permissions.cpp
@@ -3,9 +3,9 @@
 // RUN: umask 000
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s
-// CHECK000: rw-rw-rw-
+// CHECK000: rw-rw-{{rw-|---}}
 
 // RUN: umask 002
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK002 %s
-// CHECK002: rw-rw-r--
+// CHECK002: rw-rw-{{r--|---}}


Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ clang/test/Misc/permissions.cpp
@@ -3,9 +3,9 @@
 // RUN: umask 000
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s
-// CHECK000: rw-rw-rw-
+// CHECK000: rw-rw-{{rw-|---}}
 
 // RUN: umask 002
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK002 %s
-// CHECK002: rw-rw-r--
+// CHECK002: rw-rw-{{r--|---}}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/test/Misc/permissions.cpp:8
 
 // RUN: umask 002
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t

If you change this to `umask 022`, does that result in `rw-r-`? That would 
make the test meaningful on your system.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 3 inline comments as done.
aganea added inline comments.



Comment at: clang/test/Misc/permissions.cpp:8
 
 // RUN: umask 002
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t

rnk wrote:
> If you change this to `umask 022`, does that result in `rw-r-`? That 
> would make the test meaningful on your system.
No, using `umask 022` has no effect, still yields 'rw':
```
$ umask

$ umask 0077
$ touch test
$ ls -l
-rw-rw 1 aganea sudosgroup0 Dec  3 11:02 test
```
So this seems to be related to the interaction between ACL and `umask`. The [[ 
http://man7.org/linux/man-pages/man2/umask.2.html | umask doc ]] says they 
should interact, but that doesn't seem to work on my Ubuntu 18.04.01. No matter 
what I set in the umask mode, creating a new file inherits the default ACL.
All my folders have ACL enabled:
```
$ ls -l /mnt/
drwxrws--x+ 8 root sudosgroup 4096 Dec  3 17:07 f   <-- note the + which 
indicates ACL is being used
```

I could give 'rw' permissions to others:
```
$ setfacl -R -d -m o::rw /mnt/f
```
However even with that, the test fails (because umask has no effect).
I'm not sure what the right fix would be here. I can investigate other things. 
Any suggestions?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: clang/test/Misc/permissions.cpp:8
 
 // RUN: umask 002
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t

aganea wrote:
> rnk wrote:
> > If you change this to `umask 022`, does that result in `rw-r-`? That 
> > would make the test meaningful on your system.
> No, using `umask 022` has no effect, still yields 'rw':
> ```
> $ umask
> 
> $ umask 0077
> $ touch test
> $ ls -l
> -rw-rw 1 aganea sudosgroup0 Dec  3 11:02 test
> ```
> So this seems to be related to the interaction between ACL and `umask`. The 
> [[ http://man7.org/linux/man-pages/man2/umask.2.html | umask doc ]] says they 
> should interact, but that doesn't seem to work on my Ubuntu 18.04.01. No 
> matter what I set in the umask mode, creating a new file inherits the default 
> ACL.
> All my folders have ACL enabled:
> ```
> $ ls -l /mnt/
> drwxrws--x+ 8 root sudosgroup 4096 Dec  3 17:07 f   <-- note the + which 
> indicates ACL is being used
> ```
> 
> I could give 'rw' permissions to others:
> ```
> $ setfacl -R -d -m o::rw /mnt/f
> ```
> However even with that, the test fails (because umask has no effect).
> I'm not sure what the right fix would be here. I can investigate other 
> things. Any suggestions?
> I'm not sure what the right fix would be here. I can investigate other 
> things. Any suggestions?

I don't think it's worth it. Let's go with your fix and make the test pass.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 3 inline comments as done.
aganea added inline comments.



Comment at: clang/test/Misc/permissions.cpp:8
 
 // RUN: umask 002
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t

rnk wrote:
> aganea wrote:
> > rnk wrote:
> > > If you change this to `umask 022`, does that result in `rw-r-`? That 
> > > would make the test meaningful on your system.
> > No, using `umask 022` has no effect, still yields 'rw':
> > ```
> > $ umask
> > 
> > $ umask 0077
> > $ touch test
> > $ ls -l
> > -rw-rw 1 aganea sudosgroup0 Dec  3 11:02 test
> > ```
> > So this seems to be related to the interaction between ACL and `umask`. The 
> > [[ http://man7.org/linux/man-pages/man2/umask.2.html | umask doc ]] says 
> > they should interact, but that doesn't seem to work on my Ubuntu 18.04.01. 
> > No matter what I set in the umask mode, creating a new file inherits the 
> > default ACL.
> > All my folders have ACL enabled:
> > ```
> > $ ls -l /mnt/
> > drwxrws--x+ 8 root sudosgroup 4096 Dec  3 17:07 f   <-- note the + 
> > which indicates ACL is being used
> > ```
> > 
> > I could give 'rw' permissions to others:
> > ```
> > $ setfacl -R -d -m o::rw /mnt/f
> > ```
> > However even with that, the test fails (because umask has no effect).
> > I'm not sure what the right fix would be here. I can investigate other 
> > things. Any suggestions?
> > I'm not sure what the right fix would be here. I can investigate other 
> > things. Any suggestions?
> 
> I don't think it's worth it. Let's go with your fix and make the test pass.
Good. I'll add a comment above in the test to explain the situation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 232100.
aganea marked an inline comment as done.
aganea added subscribers: tstellar, Meinersbur.
aganea added a comment.

Just after I hit Submit last night, I found a better way, by disabling ACL for 
that folder.
Ping @tstellar @Meinersbur in case they have an opinion.
Please let me know which solution you think is better.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70854/new/

https://reviews.llvm.org/D70854

Files:
  clang/test/Misc/permissions.cpp


Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ clang/test/Misc/permissions.cpp
@@ -1,5 +1,9 @@
 // REQUIRES: shell
 
+// Disable ACL for the output folder, otherwise umask below will have no 
effect.
+// RUN: setfacl -bn %T
+// RUN: chmod -R o+rx %T
+
 // RUN: umask 000
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s


Index: clang/test/Misc/permissions.cpp
===
--- clang/test/Misc/permissions.cpp
+++ clang/test/Misc/permissions.cpp
@@ -1,5 +1,9 @@
 // REQUIRES: shell
 
+// Disable ACL for the output folder, otherwise umask below will have no effect.
+// RUN: setfacl -bn %T
+// RUN: chmod -R o+rx %T
+
 // RUN: umask 000
 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t
 // RUN: ls -l %t | FileCheck --check-prefix=CHECK000 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits