WIP Fix file permissions of package.py on Windows #598
Conversation
They aren't supported on Windows, and result in read-only file permissions which cannot be overwritten and breaks building of variants amongst others, see https://docs.python.org/2/library/os.html#os.chmod Cosmetics
|
This also fixes the issue on Windows whereby you cannot Before $ rez build --install
$ rez build --install
WindowsError: [Error 5] Access is denied.After $ rez build --install
$ rez build --install
$ rez build --install |
|
I can confirm this being an issue and also being a regression. It was introduced in this commit: 3c861b4#diff-104ec3e25911699423194e63a835d25d But it seems that this was intenionally added to explicitly set the file to read only. So i wonder if the fix must happen elsewhere to be really consistent? |
|
So after thinking a bit about this i think the general idea is to handle setting it readable before modifications. There seem to be one or more decorators that do that (one is @make_path_writeable). Hence i am wondering if maybe the solution should be to fix the functions that make the path writeable again instead of not making the path read-only in the first place. Personally i don't care about the path or files being read-only as we enforce permissions on the share level already, but i think this is still worth evaluating. @nerdvegas could you give some insight on the idea behind these changes? They are kind of recent. |
Agreed. Since making this PR, despite the permissions being set to writable on Windows, I've still had issues with package.py files becoming read-only from somewhere else. I'm unsure of whether it's Rez, or even Git or some Windows indexing service. It happens sporadically, and my workaround has been this. for attempt in range(2):
try:
with atomic_write(filepath, overwrite=True) as f:
f.write(content)
except WindowsError as e:
if attempt == 0:
# `overwrite=True` of atomic_write doesn't restore
# writability to the file being written to.
os.chmod(filepath, stat.S_IWRITE | stat.S_IREAD)
else:
# Under Windows, atomic_write doesn't tell you about
# which file actually failed.
raise WindowsError("%s: '%s'" % (e, filepath))Which has worked so far, but is a little backwards.. The system fighting itself. |
|
Ah. I think I'd assumed that the `atomic_write` call in
`open_file_for_write` would be doing a file rm + mv (which I think it does
on linux), and so read-only mode on the target file would not matter. I am
assuming this isn't what's happening on windows.
If I'm not mistaken, fixing it should just be a case of catching the EACCES
IOError at
https://github.com/nerdvegas/rez/blob/master/src/rez/serialise.py#L67, and
doing a chmod (make writable) and retry.
Thx
A
…On Wed, Apr 24, 2019 at 8:01 PM Marcus Ottosson ***@***.***> wrote:
Hence i am wondering if maybe the solution should be to fix the functions
that make the path writeable again instead of not making the path read-only
in the first place.
Agreed.
Since making this PR, despite the permissions being set to writable on
Windows, I've still had issues with package.py files becoming read-only
from somewhere else. I'm unsure of whether it's Rez, or even Git or some
Windows indexing service. It happens sporadically, and my workaround has
been this.
for attempt in range(2):
try:
with atomic_write(filepath, overwrite=True) as f:
f.write(content)
except WindowsError as e:
if attempt == 0:
# `overwrite=True` of atomic_write doesn't restore
# writability to the file being written to.
os.chmod(filepath, stat.S_IWRITE | stat.S_IREAD)
else:
# Under Windows, atomic_write doesn't tell you about
# which file actually failed.
raise WindowsError("%s: '%s'" % (e, filepath))
Which has worked so far, but is a little backwards.. The system fighting
itself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#598 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSQP44GRP7BSGAGFP6DPSAVYJANCNFSM4HG6IXZQ>
.
|
|
@nerdvegas That's actually what I found myself doing which has worked well so far. I pushed an update with this now, let me know if you'd like anything changed. |
|
Didn't work having just that in place, not entirely sure why. |
This hasn't been confirmed yet; on some machines, this hasn't been an issue, but on mine (Windows 10, non-privileged user, local disk) it was a showstopper.
The permission bits here aren't supported on Windows which shouldn't be a problem, as they should fall back to the only two bits that are,
IWRITEandIREAD. Here however, they result in read-only file permissions that cannot be overwritten and breaks building of variants amongst othersConfirmed with this minimal reproducible.