bitbake: Correct broken symlink behaviour

PATCH In review Roman Savchenko 2 participants 7 messages Sep 20, 2016 – Sep 27, 2016

lore

Share & tools

lore

Roman Savchenko

author

2016-09-20 16:23 UTC #0 lore raw

PATCH 1/1 bitbake: Correct broken symlink behaviour

From: Roman Savchenko <[email protected]>

Unlink broken symlink.

Signed-off-by Roman Savchenko <[email protected]>

---
 bitbake/lib/bb/fetch2/__init__.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

bitbake/lib/bb/fetch2/__init__.py +13 -8

index 6ef0c6f..a752c6c 100644
@@ -925,6 +925,17 @@ def rename_bad_checksum(ud, suffix):
     bb.utils.movefile(ud.localpath, new_localpath)
 
 
+def try_symlink(source, link_name):
+    """
+    Tries to make link to passed source if link name does not
+    exist.
+    """
+    if not os.path.exists(link_name):
+        if os.path.islink(link_name):
+            os.unlink(link_name)
+
+        os.symlink(source, link_name)
+
 def try_mirror_url(fetch, origud, ud, ld, check = False):
     # Return of None or a value means we're finished
     # False means try another url

@@ -962,20 +973,14 @@ def try_mirror_url(fetch, origud, ud, ld, check = False):
                 bb.utils.mkdirhier(os.path.dirname(ud.donestamp))
                 open(ud.donestamp, 'w').close()
             dest = os.path.join(dldir, os.path.basename(ud.localpath))
-            if not os.path.exists(dest):
-                os.symlink(ud.localpath, dest)
+            try_symlink(ud.localpath, dest)
             if not verify_donestamp(origud, ld) or origud.method.need_update(origud, ld):
                 origud.method.download(origud, ld)
                 if hasattr(origud.method,"build_mirror_data"):
                     origud.method.build_mirror_data(origud, ld)
             return origud.localpath
         # Otherwise the result is a local file:// and we symlink to it
-        if not os.path.exists(origud.localpath):
-            if os.path.islink(origud.localpath):
-                # Broken symbolic link
-                os.unlink(origud.localpath)
-
-            os.symlink(ud.localpath, origud.localpath)
+        try_symlink(ud.localpath, origud.localpath)
         update_stamp(origud, ld)
         return ud.localpath

Signature · 1 line

2.7.4

Richard Purdie

2016-09-21 21:17 UTC #1 lore raw

Hi,

Quoted 3 lines · Roman Savchenko

From: Roman Savchenko <[email protected]>
Unlink broken symlink.

This really needs a few more details about the problem and how an issue occurs. I can understand from reading the patch but not from the commit message.

Quoted 15 lines

Signed-off-by Roman Savchenko <[email protected]>
---
bitbake/lib/bb/fetch2/__init__.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
bitbake/lib/bb/fetch2/__init__.py +0 -0
index 6ef0c6f..a752c6c 100644
@@ -925,6 +925,17 @@ def rename_bad_checksum(ud, suffix):
      bb.utils.movefile(ud.localpath, new_localpath)
bitbake/lib/bb/fetch2/__init__.py +1 -0
+def try_symlink(source, link_name):

bitbake/lib/bb/fetch2/__init__.py def rename_bad_checksum(ud, suffix): · line 938

Quoted 10 lines

bitbake/lib/bb/fetch2/__init__.py +10 -0

+    """
+    Tries to make link to passed source if link name does not
+    exist.
+    """
+    if not os.path.exists(link_name):
+        if os.path.islink(link_name):
+            os.unlink(link_name)
+
+        os.symlink(source, link_name)
+

I don't think the function name is right. "try" implies its fine if it doesn't work out. In reality its more like "symlink if its invalid". "create_valid_symlink" whilst long is probably more reflective of what it does but I'm sure we could still do better :/.

You could add "or the symlink is invalid" to the function description too.

Cheers,

Richard

Roman Savchenko

author

2016-09-23 07:46 UTC #2 lore raw

PATCH 1/1 bitbake: Correct broken symlink behaviour

In reply to Roman Savchenko

From: Roman Savchenko <[email protected]>

When making new symlink in case when link name does not exist possible broken symlink behaviour should be taken into account.

Signed-off-by Roman Savchenko <[email protected]>

---
 bitbake/lib/bb/fetch2/__init__.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

bitbake/lib/bb/fetch2/__init__.py +13 -8

index 6ef0c6f..d8a3b96 100644
@@ -925,6 +925,17 @@ def rename_bad_checksum(ud, suffix):
     bb.utils.movefile(ud.localpath, new_localpath)
 
 
+def try_symlink(source, link_name):
+    """
+    Tries to make link to passed source if link name does not
+    exist or it is the invalid symlink.
+    """
+    if not os.path.exists(link_name):
+        if os.path.islink(link_name):
+            os.unlink(link_name)
+
+        os.symlink(source, link_name)
+
 def try_mirror_url(fetch, origud, ud, ld, check = False):
     # Return of None or a value means we're finished
     # False means try another url

@@ -962,20 +973,14 @@ def try_mirror_url(fetch, origud, ud, ld, check = False):
                 bb.utils.mkdirhier(os.path.dirname(ud.donestamp))
                 open(ud.donestamp, 'w').close()
             dest = os.path.join(dldir, os.path.basename(ud.localpath))
-            if not os.path.exists(dest):
-                os.symlink(ud.localpath, dest)
+            try_symlink(ud.localpath, dest)
             if not verify_donestamp(origud, ld) or origud.method.need_update(origud, ld):
                 origud.method.download(origud, ld)
                 if hasattr(origud.method,"build_mirror_data"):
                     origud.method.build_mirror_data(origud, ld)
             return origud.localpath
         # Otherwise the result is a local file:// and we symlink to it
-        if not os.path.exists(origud.localpath):
-            if os.path.islink(origud.localpath):
-                # Broken symbolic link
-                os.unlink(origud.localpath)
-
-            os.symlink(ud.localpath, origud.localpath)
+        try_symlink(ud.localpath, origud.localpath)
         update_stamp(origud, ld)
         return ud.localpath

Signature · 1 line

2.7.4

Roman Savchenko

author

2016-09-23 07:55 UTC #3 lore raw

Hi Richard,

I adjusted commit message and function description. but I do not agree with proposed
function name create_valid_symlink, becasue link creation could fail - so I left try_symlink
to point this behaviour.

Regards,
Roman

2016-09-23 10:45 GMT+03:00 Roman Savchenko <[email protected]>:

Quoted 62 lines

From: Roman Savchenko <[email protected]>
When making new symlink in case when link name does not exist possible broken symlink behaviour should be taken into account.
Signed-off-by Roman Savchenko <[email protected]>
---
 bitbake/lib/bb/fetch2/__init__.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
bitbake/lib/bb/fetch2/__ renamed from bitbake/lib/bb/fetch2/__init__.py +0 -0
init__.py
index 6ef0c6f..d8a3b96 100644
bitbake/lib/bb/fetch2/__init__.py +11 -0
@@ -925,6 +925,17 @@ def rename_bad_checksum(ud, suffix):
     bb.utils.movefile(ud.localpath, new_localpath)
 
 
+def try_symlink(source, link_name):
+    """
+    Tries to make link to passed source if link name does not
+    exist or it is the invalid symlink.
+    """
+    if not os.path.exists(link_name):
+        if os.path.islink(link_name):
+            os.unlink(link_name)
+
+        os.symlink(source, link_name)
+
 def try_mirror_url(fetch, origud, ud, ld, check = False):
     # Return of None or a value means we're finished
     # False means try another url
@@ -962,20 +973,14 @@ def try_mirror_url(fetch, origud, ud, ld, check =
False):
bb.utils.mkdirhier(os.path.dirname(ud.donestamp)) open(ud.donestamp, 'w').close()
dest = os.path.join(dldir, os.path.basename(ud.localpath))
bitbake/lib/bb/fetch2/__init__.py +1 -2
-            if not os.path.exists(dest):
-                os.symlink(ud.localpath, dest)
+            try_symlink(ud.localpath, dest)
             if not verify_donestamp(origud, ld) or
origud.method.need_update(origud, ld):
origud.method.download(origud, ld)
if hasattr(origud.method,"build_mirror_data"):
origud.method.build_mirror_data(origud, ld)
return origud.localpath
# Otherwise the result is a local file:// and we symlink to it
bitbake/lib/bb/fetch2/__init__.py +1 -7
-        if not os.path.exists(origud.localpath):
-            if os.path.islink(origud.localpath):
-                # Broken symbolic link
-                os.unlink(origud.localpath)
-
-            os.symlink(ud.localpath, origud.localpath)
+        try_symlink(ud.localpath, origud.localpath)
         update_stamp(origud, ld)
         return ud.localpath
 
--
2.7.4

Richard Purdie

2016-09-23 14:01 UTC #4 lore raw

Hi Roman,

Quoted 5 lines · Roman Savchenko

I adjusted commit message and function description. but I do not agree with proposed
function name create_valid_symlink, becasue link creation could fail - so I left try_symlink
to point this behaviour.

I don't agree with "try_symlink" though, sorry.

Link creation can fail but that is like any other function which can also fail. We don't prefix all function names with try_ because they can fail.

We need a better name.

Cheers,

Richard

Roman Savchenko

author

2016-09-27 10:22 UTC #5 lore raw

PATCH 1/1 bitbake: Correct broken symlink behaviour

In reply to Roman Savchenko

From: Roman Savchenko <[email protected]>

When making new symlink in case when link name does not exist possible broken symlink behaviour should be taken into account.

Signed-off-by Roman Savchenko <[email protected]>

---
 bitbake/lib/bb/fetch2/__init__.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

bitbake/lib/bb/fetch2/__init__.py +13 -8

index 6ef0c6f..6638b34 100644
@@ -925,6 +925,17 @@ def rename_bad_checksum(ud, suffix):
     bb.utils.movefile(ud.localpath, new_localpath)
 
 
+def create_symlink_if_not_exist(source, link_name):
+    """
+    Tries to make link to passed source if link name does not
+    exist or it is the invalid symlink.
+    """
+    if not os.path.exists(link_name):
+        if os.path.islink(link_name):
+            os.unlink(link_name)
+
+        os.symlink(source, link_name)
+
 def try_mirror_url(fetch, origud, ud, ld, check = False):
     # Return of None or a value means we're finished
     # False means try another url

@@ -962,20 +973,14 @@ def try_mirror_url(fetch, origud, ud, ld, check = False):
                 bb.utils.mkdirhier(os.path.dirname(ud.donestamp))
                 open(ud.donestamp, 'w').close()
             dest = os.path.join(dldir, os.path.basename(ud.localpath))
-            if not os.path.exists(dest):
-                os.symlink(ud.localpath, dest)
+            create_symlink_if_not_exist(ud.localpath, dest)
             if not verify_donestamp(origud, ld) or origud.method.need_update(origud, ld):
                 origud.method.download(origud, ld)
                 if hasattr(origud.method,"build_mirror_data"):
                     origud.method.build_mirror_data(origud, ld)
             return origud.localpath
         # Otherwise the result is a local file:// and we symlink to it
-        if not os.path.exists(origud.localpath):
-            if os.path.islink(origud.localpath):
-                # Broken symbolic link
-                os.unlink(origud.localpath)
-
-            os.symlink(ud.localpath, origud.localpath)
+        create_symlink_if_not_exist(ud.localpath, origud.localpath)
         update_stamp(origud, ld)
         return ud.localpath

Signature · 1 line

2.7.4

Roman Savchenko

author

2016-09-27 10:24 UTC #6 lore raw

Hi Richard,

I renamed function, probably name is a little bit long, but it fully displays what it does.

Regards,
Roman.

2016-09-27 13:22 GMT+03:00 Roman Savchenko <[email protected]>:

Quoted 62 lines

From: Roman Savchenko <[email protected]>
When making new symlink in case when link name does not exist possible broken symlink behaviour should be taken into account.
Signed-off-by Roman Savchenko <[email protected]>
---
 bitbake/lib/bb/fetch2/__init__.py | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
bitbake/lib/bb/fetch2/__ renamed from bitbake/lib/bb/fetch2/__init__.py +0 -0
init__.py
index 6ef0c6f..6638b34 100644
bitbake/lib/bb/fetch2/__init__.py +11 -0
@@ -925,6 +925,17 @@ def rename_bad_checksum(ud, suffix):
     bb.utils.movefile(ud.localpath, new_localpath)
 
 
+def create_symlink_if_not_exist(source, link_name):
+    """
+    Tries to make link to passed source if link name does not
+    exist or it is the invalid symlink.
+    """
+    if not os.path.exists(link_name):
+        if os.path.islink(link_name):
+            os.unlink(link_name)
+
+        os.symlink(source, link_name)
+
 def try_mirror_url(fetch, origud, ud, ld, check = False):
     # Return of None or a value means we're finished
     # False means try another url
@@ -962,20 +973,14 @@ def try_mirror_url(fetch, origud, ud, ld, check =
False):
bb.utils.mkdirhier(os.path.dirname(ud.donestamp)) open(ud.donestamp, 'w').close()
dest = os.path.join(dldir, os.path.basename(ud.localpath))
bitbake/lib/bb/fetch2/__init__.py +1 -2
-            if not os.path.exists(dest):
-                os.symlink(ud.localpath, dest)
+            create_symlink_if_not_exist(ud.localpath, dest)
             if not verify_donestamp(origud, ld) or
origud.method.need_update(origud, ld):
origud.method.download(origud, ld)
if hasattr(origud.method,"build_mirror_data"):
origud.method.build_mirror_data(origud, ld)
return origud.localpath
# Otherwise the result is a local file:// and we symlink to it
bitbake/lib/bb/fetch2/__init__.py +1 -7
-        if not os.path.exists(origud.localpath):
-            if os.path.islink(origud.localpath):
-                # Broken symbolic link
-                os.unlink(origud.localpath)
-
-            os.symlink(ud.localpath, origud.localpath)
+        create_symlink_if_not_exist(ud.localpath, origud.localpath)
         update_stamp(origud, ld)
         return ud.localpath
 
--
2.7.4