Drop accidental write-attributes request
ClosedPublic

Authored by Phyx on Sep 24 2018, 1:05 AM.

Details

Summary

The new filesystem code accidentally asks for write attributes permissions when doing read-only access.

I believe this is what's causing the GHC 8.6.1 tarballs to fail when installed to a privileged location.
I haven't been able to reproduce the issue yet, but this permission bit is wrong anyway.

Test Plan

I'm still trying to workout how to test that this works, changing the permissions on the folder doesn't seem to reproduce the error on a tarball I made from before the change.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Phyx created this revision.Sep 24 2018, 1:05 AM

Could you rebase? This should fix the build failure.

tdammers requested changes to this revision.Oct 1 2018, 8:25 AM
tdammers added a subscriber: tdammers.

Is there any test that reflects this change?

This revision now requires changes to proceed.Oct 1 2018, 8:25 AM
Phyx requested review of this revision.Oct 2 2018, 2:06 PM

Is there any test that reflects this change?

@tdammers No, as I mentioned in the summary, it's hard to test for. It's impossible to test for in the CI. This requires a file for which you are not the owner and have been granted read only permission to.
Which by definition means you'll need multiple users and the ability to to modify ACL bits. So you'll either have to elevate or have credentials for multiple users.

Unfortunately no there's a good reason we've missed this up until now...

@monoidal I'll rebase it as soon as I finish some changes to the branch it's on (the patches are stacked on top of eachother). Though I'm not really worried about the CI for this patch.

Phyx updated this revision to Diff 18190.Oct 3 2018, 1:13 AM
Phyx edited the summary of this revision. (Show Details)
  • rebase and checked rest attributes
bgamari accepted this revision.Oct 3 2018, 5:01 AM

Alright, this looks fine to me.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 3 2018, 10:17 AM
This revision was automatically updated to reflect the committed changes.

@bgamari This can be merged to 8.6.