Ensure that README.txt has write permissions for subsequent imports. #5249
Conversation
|
This happens because you are using a system installation for which you do not have write permissions to the install location of scikit image? This is a pretty esoteric edge case (but a very important one) could you please add a comment in the source code that reiterates why it is important to update permissions. Thanks! |
|
Yes, exactly the case. This has come up when using the |
|
I ran into this exact issue that this PR is fixing just now... In my case, the situation was as follows:
This then resulted in errors like this when importing So very much +1 on this proposed change! |
skimage/data/__init__.py
Outdated
| # we need to ensure README.txt has user-write permission when it is | ||
| # put into their cache directory. | ||
| current = stat.S_IMODE(os.lstat(dest_path).st_mode) | ||
| os.chmod(dest_path, current | stat.S_IWUSR) |
There was a problem hiding this comment.
Another option would be to check if the dest_path already exists, and only copy it if it doesn't.
Unless there's a particular reason why every import skimage needs to result in copying the README.txt file?
There was a problem hiding this comment.
I agree, but this leaves the ability to update the README.txt ... then again, if the cache directory has the version in it, then this shouldn't happen, I just now realize.
There was a problem hiding this comment.
I would like to hear from one of the scikit-image maintainers ... if there's a reason to recopy, we can leave this, or I'd be happy to change it to a check if the file exists. (Which is probably more efficient, although it's a tiny file.)
There was a problem hiding this comment.
it might just be better to read the file line by line, and just overwrite the file. The README.txt file in the data cache directory can be assume to be owned by us.
Reading and writing files with the default settings enables system admins to choose good defaults for their security model.
There was a problem hiding this comment.
Is there a reason it has to be overwritten, though? Can't we just check that it exists and not copy/overwrite if it's there? I'm of a mind that would be the most reliable and fastest.
There was a problem hiding this comment.
I'm worried that the text won't get updated if we change the language.
I think you can skip the copy if the file exists for now. Please justify your decision in a comment in the code.
I'm rather agnostic for this small file.
Do we copy in any other files with shutil?
There was a problem hiding this comment.
I have updated to only copy if it isn't there, and added an explanation. In general, I don't think this is a problem because the cache directory contains the scikit-image version number, and I can't see a path that README.txt would change within a given version/installation without shenanigans.
As for other copies, yes, the other data files are all copied in. However, they have their hash checked so will not be re-copied if their hashes match. It is possible that somebody could make a file writeable, modify it, and put it back to read only and then one of these copy2 actions would fail, but that is hard to protect against. As for the files themselves changing from version to version, that is possible but they would be put into a different cache directory bypassing the overwrite problem.
|
I'm OK with this for the same reasons you listed. We have version numbers in our cache, so that should make sure the readme is mostly up to date. Hashing the readme is somewhat strange. Thanks for addressing this! |
|
Thanks @erykoff! |
…cikit-image#5249) * Ensure that README.txt has write permissions for subsequent imports. * Add comment * Only copy the README.txt file if it isn't there.
…cikit-image#5249) * Ensure that README.txt has write permissions for subsequent imports. * Add comment * Only copy the README.txt file if it isn't there.
Description
This PR fixes a bug in which if
data/README.txthas been made read-only in the source directory, then the first import ofskimageworks because the file is copied into the.cache/scikit-image/directory successfully. But subsequent imports will fail becausecopy2will not be able to copyREADME.txtonto a read-only target. This PR fixes the problem by ensuring thatREADME.txthas user permissions as a writable file.Checklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.