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

Make sure copied files actually have write permissions in ~/.openshot-qt #2972

Closed
peti opened this issue Sep 3, 2019 · 5 comments · Fixed by #2973
Closed

Make sure copied files actually have write permissions in ~/.openshot-qt #2972

peti opened this issue Sep 3, 2019 · 5 comments · Fixed by #2973
Labels
🐞 bug A bug, error, or breakage of any kind

Comments

@peti
Copy link

peti commented Sep 3, 2019

openshot-qt copies all kinds of Python or SVG files into a temporary location, i.e.~/.openshot_qt/blender/8f9c347a-ce4e-11e9-b006-a4c3f0b51071/lens_flare.py. Now, on the NixOS those files are installed with 0444 permissions. The originals have no +w bit -- and consequently the copy lacks that permission and cannot be used properly by the edit. This trivial problem breaks the title generator on NixOS, among other things (NixOS/nixpkgs#48591).

Would you consider adding code into oneshot-qt that simply performs a chmod u+w on the newly created temporary file after copying it?

@ferdnyc
Copy link
Contributor

ferdnyc commented Sep 3, 2019

@peti The interesting thing is, we're not creating that file as a copy of the source. We're reading the source file into memory, then writing it out to a newly-created file. So, the permissions of the original shouldn't matter, I don't think. If it's being created read-only, I suspect it probably has something to do with the umask on your system. (Out of curiosity, are the .png files created by Blender also read-only?)

Whatever the reason, I've submitted #2973 with a change to explicitly set write permission on the generated .py file after it's created. That should hopefully ensure that the file is made writable on all three OSes.

@ferdnyc ferdnyc added ❓ needs info Issue needs additional information to be useful (such as missing logs, missing steps, etc...) needs testing Issue or PR needs some help testing from the community or other developers pending A PR has been submitted to fix the issue labels Sep 3, 2019
@ferdnyc
Copy link
Contributor

ferdnyc commented Sep 3, 2019

Aagh! Never mind, I was wrong. We do copy the file first, before modifying it.

But that actually makes things even easier, we can use Python's shutil.copyfile() instead of shutil.copy(), to copy the file without duplicating its permissions. Ignore that AppImage, I'm going to update the PR to fix both the animated and still title file copying.

@ferdnyc ferdnyc removed ❓ needs info Issue needs additional information to be useful (such as missing logs, missing steps, etc...) needs testing Issue or PR needs some help testing from the community or other developers labels Sep 3, 2019
@ferdnyc
Copy link
Contributor

ferdnyc commented Sep 3, 2019

OK, #2973 should now fix both the .svg files and the .py files. Don't worry about testing it, though, as the AppImage won't be a valid test anymore ­— its template files are most likely not set read-only.

However, if you want to attempt a local fix, the fix is as simple as changing shutil.copy() to shutil.copyfile() in both openshot_qt/windows/views/blender_listview.py and openshot_qt/windows/title_editor.py in your OpenShot install.

peti added a commit to NixOS/nixpkgs that referenced this issue Sep 3, 2019
@ferdnyc has kindly provided a patch for our Nix-specific permission
issue OpenShot/openshot-qt#2972.

Fixes #32898.
Fixes #48591.
Related to #55683.
@peti
Copy link
Author

peti commented Sep 3, 2019

I applied the patch in NixOS (NixOS/nixpkgs@d7bbcdf) and the new version appears to work fine. Thank you very much for the quick fix!

@peti peti closed this as completed Sep 3, 2019
@ferdnyc
Copy link
Contributor

ferdnyc commented Sep 3, 2019

Glad to hear that worked! Reopening this only because the fix hasn't gone in yet on our end; GitHub will auto-close when the PR gets merged.

@ferdnyc ferdnyc reopened this Sep 3, 2019
@ferdnyc ferdnyc added merged and removed pending A PR has been submitted to fix the issue labels Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug A bug, error, or breakage of any kind
Projects
None yet
2 participants