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

Fix: conan config install - overwrite read-only files / don't copy file permissions #7004

Merged
merged 4 commits into from May 13, 2020

Conversation

e-i-n-s
Copy link
Contributor

@e-i-n-s e-i-n-s commented May 12, 2020

Changelog: Fix: conan config install can overwrite read-only files and won't copy permissions.
Docs: omit

  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.

Issue when running conan config install [...] (existing settings.yml is read-only!)

Installing settings.yml
Traceback (most recent call last):
 File "conan\conans\client\command.py", line 1607, in run
 File "conan\conans\client\command.py", line 478, in config
 File "conan\conans\client\conan_api.py", line 92, in wrapper
 File "conan\conans\client\conan_api.py", line 621, in config_install
 File "conan\conans\client\conf\config_installer.py", line 230, in configuration_install
 File "conan\conans\client\conf\config_installer.py", line 182, in _process_config
 File "conan\conans\client\conf\config_installer.py", line 85, in _process_folder
 File "shutil.py", line 241, in copy
 File "shutil.py", line 121, in copyfile
PermissionError: [Errno 13] Permission denied: 'D:\\TC\\work\\3895a438cac8f82f\\.conan\\settings.yml

In Linux the issue doesn't occur.
In my case the cause was a read only permission in the "source" configuration directory. In my mind conan config install should overwrite read-only files (in conan cache) and shouldn't copy file permissions.

os.remove(dst)
shutil.copy(src, dst)
shutil.copyfile(src, dst)
Copy link
Member

@memsharded memsharded May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned that this might break for other users. We had the case of @fulara that used different users (the reason the above os.remove() was introduced). copyfile copy with permissions, something that was avoided.

Isn't it possible to achieve the same without changing this line?

Copy link
Contributor

@fulara fulara May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in my origianl request:
#6556

I explained how in my opinion the copyfile would resolve my issue,
it is because the copyfile unlike copy does not change the owner of the file, it just replaces the contents.

In this case though the file is removed if it exists so this should be fine.
Hence the chnge should be fine in my opinion.

Copy link
Contributor Author

@e-i-n-s e-i-n-s May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned that this might break for other users. We had the case of @fulara that used different users (the reason the above os.remove() was introduced). copyfile copy with permissions, something that was avoided.

Isn't it possible to achieve the same without changing this line?

shutil.copyfile copies without permissions :-)

shutil:

copy() copies the file data and the file’s permission mode (see os.chmod()). Other metadata, like the file’s creation and modification times, is not preserved. To preserve all file metadata from the original, use copy2() instead.

copyfile(): Copy the contents (no metadata) of the file [...]

Source: https://docs.python.org/3.7/library/shutil.html

@e-i-n-s e-i-n-s requested a review from memsharded May 13, 2020
@memsharded memsharded added this to the 1.26 milestone May 13, 2020
@memsharded memsharded merged commit 364e2ce into conan-io:develop May 13, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants