Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isort should not change the uid/gid when writing a file #1638

Closed
taxonomic-blackfish opened this issue Jan 18, 2021 · 5 comments
Closed

isort should not change the uid/gid when writing a file #1638

taxonomic-blackfish opened this issue Jan 18, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@taxonomic-blackfish
Copy link

If isort is run against a file, it will change the owner of the file to the uid/gid of the user running the command rather than the original owner of the file. It would be useful to attempt to retain original permissions. This is of particular concern when running dockerized dev containers with formatting tools installed that may run as a user other that of the host machine.

For comparison, the tool black does not appear to have this issue when run in the same contexts.

Example:

# stat -c %u:%g ./foo.py
1000:1000
# isort ./foo.py
Fixing /src/foo.py
# stat -c %u:%g ./foo.py
0:0

The expected result is the final stat returns 1000:1000.

@timothycrosley
Copy link
Member

isort does make a best effort attempt to do this, that seems to work in most cases for most people:
https://github.com/PyCQA/isort/blob/develop/isort/api.py#L355

That said I'm open to more ideas about how to improve this, or information that is leading this not to work in your case such as potentially a different run mode. Testing the same example across users on my own machine shows that for my use the uid and gid are preserved.

isort and black are not really comparable in this regard because black works by loading the full file in memory and then writing to the same file in place. isort uses a streaming approach so it needs to write to a different file, while streaming in the original line by line. It then swaps the temp file over the old file, this makes the gid and uid question more complex but means isort can work against much larger files and consume a constant amount of memory.

@timothycrosley timothycrosley added the repo_needed Can't currently reproduce, if reproduction steps are added we will resivit this issue. label Jan 18, 2021
@taxonomic-blackfish
Copy link
Author

I don't know how either work under the hood. It might be possible to stat the file beforehand and opportunistically attempt to set the uid/gid from these values. I'd guess a majority of cases would be this happening with root vs. non-root users as show in the example. A warning could be emitted (or a flag could cause non-zero exit too) if the uid/gid couldn't be set.

@gofr
Copy link
Contributor

gofr commented Feb 9, 2021

shutil.copymode only copies the permission bits, not the user/group.

os.stat + os.chown could fix this according to:
https://stackoverflow.com/questions/19787348/copy-file-keep-permissions-and-owner

os.chown doesn't exist on Windows, though.

@gofr
Copy link
Contributor

gofr commented Feb 9, 2021

And I can reproduce it with something like this:

echo -e "import foo\nimport bar\n" > test_ownership.py
sudo chown nobody test_ownership.py
isort test_ownership.py

After that, the file is owned by me, not by nobody.

But as the above example already shows. I needed sudo to change the ownership. Only root can change the ownership. Doing os.chown would fail with an "Operation not permitted" error here. I guess it would be possible to just ignore exceptions so it works in @taxonomic-blackfish 's case where isort is run as root? I don't know if that's really a good idea.

timothycrosley added a commit that referenced this issue Mar 5, 2021
@timothycrosley timothycrosley added enhancement New feature or request and removed repo_needed Can't currently reproduce, if reproduction steps are added we will resivit this issue. labels Mar 5, 2021
@timothycrosley
Copy link
Member

Sorry for the delay here! There is now a work around in develop that will make its way out in the 5.8.0 release.
A new flag / config option (--overwrite-in-place/overwrite_in_place) will tell isort to reuse the same file handler instead of swapping it out for the streamed to output file. It hurts performance a bit and consumes extra memory, but it does ensure none of the file info will change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants