Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Added symlinks option to the copy plugin #209
Conversation
|
@femdom thank you for the PR! Please check out our contributing guidelines and make sure you've completed all the steps. In particular:
Thank you! |
|
@kyrofa, thanks! I remember. Just wanted to know if you need this change. |
|
Here is the launchpad bug page |
sergiusens
reviewed
Jan 14, 2016
| @@ -34,6 +34,13 @@ def schema(cls): | ||
| 'files': { | ||
| 'type': 'object', | ||
| }, | ||
| + 'symlinks': { |
sergiusens
Jan 14, 2016
Collaborator
how about follow-symlinks? It has a more semantic meaning for a boolean
kyrofa
reviewed
Jan 20, 2016
| @@ -57,6 +64,6 @@ def build(self): | ||
| os.makedirs(os.path.dirname(dst), exist_ok=True) | ||
| if os.path.isdir(src): | ||
| - shutil.copytree(src, dst) | ||
| + shutil.copytree(src, dst, symlinks=self.options.follow_symlinks) |
kyrofa
Jan 21, 2016
Member
Quick question: If the symlink is pointing outside of the tree, this will create a .snap with dangling symlinks. Right? Reading the docs it sounds like ignore_dangling_symlinks only has an affect if symlinks is false. I suspect that's why this wasn't done in the first place. Perhaps we can get around this by creating a custom copy function and using the copy_function parameter so we can make sure and follow symlinks that point outside the .snap?
femdom
Jan 22, 2016
Contributor
Sorry, my mistake. Follow symlniks - mean "copy file where symlink points"
Let me update a PR then.
kyrofa
Jan 22, 2016
Member
@femdom good catch, I missed that switch.
@sergiusens, @elopio we could use your input on the symlinks potentially reaching out of the .snap. Should we make sure that doesn't happen with a smarter copy function, or allow them to shoot themselves in the foot if they don't want to follow symlinks?
elopio
Jan 25, 2016
Member
I'm +1 on doing sanity checks for the resulting snap and/or preventing it from generating stupid things.
elopio
reviewed
Jan 25, 2016
| + | ||
| + os.makedirs('src') | ||
| + open('src/file.txt', 'w').close() | ||
| + os.symlink('file.txt', 'src/link.txt') |
|
@kyrofa, is showing warning enough when symlink points outside the snap, or you want to stop build process on that case? |
snappy-m-o
commented
Mar 3, 2016
|
Can one of the admins verify this patch? |
snappy-m-o
commented
Mar 3, 2016
|
Can one of the admins verify this patch? |
snappy-m-o
commented
Mar 3, 2016
|
Can one of the admins verify this patch? |
|
@femdom actually we'd like to redesign this bit of the copy plugin completely. Rather than support following symlinks and not following symlinks with an option, we'd rather get rid of the options and copy in the most efficient manner possible. Something like:
This would be the best of both worlds, would it not? Is there a downside? |
|
@kyrofa I'm not sure about the last step. What if it's pointing to the something like /etc/timezone? What if it's absolute symlink - copy it as a symlink? |
|
@femdom if the final .snap contains a symlink to /etc/timezone it won't pass the automated review tools anyway (relevant chunk of the review tools). If it's an absolute symlink or pointing outside the snap, follow the symlink and copy what it's pointing to, not the symlink itself. There are a few exceptions that the review tools will allow, such as symlinks to libc-- think we'd run into any of those with the copy plugin? Even if we do, that would be a problem with the current plugin as well, so I still say this would be an improvement. |
|
@femdom I'll close this for now. Please reopen (or create a new one) if you'd like to continue on this. |
femdom commentedJan 8, 2016
We need this change since we need to copy large directories.
Please, consider adding this change to your codebase.
Thank you,
Renat