Merge lp:~longsleep/snapcraft/snapcraft-dirs-symlink into lp:~snappy-dev/snapcraft/core

Proposed by Simon Eisenmann
Status: Merged
Approved by: Ricardo Salveti
Approved revision: 130
Merged at revision: 131
Proposed branch: lp:~longsleep/snapcraft/snapcraft-dirs-symlink
Merge into: lp:~snappy-dev/snapcraft/core
Diff against target: 22 lines (+4/-1)
1 file modified
snapcraft/plugins/ubuntu.py (+4/-1)
To merge this branch: bzr merge lp:~longsleep/snapcraft/snapcraft-dirs-symlink
Reviewer Review Type Date Requested Status
Leo Arias (community) Approve
Michael Terry (community) Approve
Review via email: mp+267176@code.launchpad.net

Commit message

Fix symlinks for directories as well.

Description of the change

Fix symlinks for directories as well.

To post a comment you must log in.
Revision history for this message
Michael Terry (mterry) wrote :

Guh, good catch! I could have sworn I tested that. :(

This method needs a unit test. But I won't block your MP fix on that.

review: Approve
Revision history for this message
Michael Terry (mterry) wrote :

Wait... This is probably a trivial change, in legal terms. But I'm not 100% on that. Plus your other branch isn't trivial.

Have you signed the contributor agreement yet? I don't see you as a member of ~contributor-agreement-canonical.

http://www.ubuntu.com/legal/contributors

Revision history for this message
Michael Terry (mterry) wrote :

<longsleep> mterry: I gave the contributor agreement to my boss and he will eventually sign it. I think the symlink patch is trivial and you can just merge that. I will let you know when the agreement is signed. Let me know if you have any further comments to the debs plugin.

Alright, will merge this as trivial.

Revision history for this message
Leo Arias (elopio) wrote :

A test here would be nice. Do you have a simple scenario that reproduces the problem?
Drop me a line on IRC and I can give you a hand writing the test. I'm elopio.

Revision history for this message
Leo Arias (elopio) wrote :

Feel free to land this first, and then the fixed test from https://code.launchpad.net/~elopio/snapcraft/symlinks-2/+merge/267240

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'snapcraft/plugins/ubuntu.py'
--- snapcraft/plugins/ubuntu.py 2015-08-06 01:00:44 +0000
+++ snapcraft/plugins/ubuntu.py 2015-08-06 11:20:50 +0000
@@ -14,6 +14,7 @@
14# You should have received a copy of the GNU General Public License14# You should have received a copy of the GNU General Public License
15# along with this program. If not, see <http://www.gnu.org/licenses/>.15# along with this program. If not, see <http://www.gnu.org/licenses/>.
1616
17import itertools
17import logging18import logging
18import os19import os
19import subprocess20import subprocess
@@ -119,7 +120,9 @@
119 have that, so instead clean those absolute symlinks."""120 have that, so instead clean those absolute symlinks."""
120 debdir = debdir or self.builddir121 debdir = debdir or self.builddir
121 for root, dirs, files in os.walk(debdir):122 for root, dirs, files in os.walk(debdir):
122 for entry in files:123 # Symlinks to directories will be in dirs, while symlinks to
124 # non-directories will be in files.
125 for entry in itertools.chain(files, dirs):
123 path = os.path.join(root, entry)126 path = os.path.join(root, entry)
124 if os.path.islink(path) and os.path.isabs(os.readlink(path)):127 if os.path.islink(path) and os.path.isabs(os.readlink(path)):
125 target = os.path.join(debdir, os.readlink(path)[1:])128 target = os.path.join(debdir, os.readlink(path)[1:])

Subscribers

People subscribed via source and target branches