sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

Submitted by Mikko Rapeli on July 31, 2015, 9 a.m. | Patch ID: 99107

Details

Message ID 1438333215-1593-1-git-send-email-mikko.rapeli@bmw.de
State New
Headers show

Commit Message

Mikko Rapeli July 31, 2015, 9 a.m.
This change makes broken symlinks stand out clearly instead of bitbake
failing with odd error messages. Tested locally with broken symlink
as SSTATE_DIR, DL_DIR and SSTATE_MIRROR.

Change-Id: I2e92702237ab3bdb897d0bdefcf33480aabbc288
Signed-off-by: Mikko Rapeli <mikko.rapeli@bmw.de>
---
 meta/classes/sanity.bbclass | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
index 5be5efb..45ca992 100644
--- a/meta/classes/sanity.bbclass
+++ b/meta/classes/sanity.bbclass
@@ -253,6 +253,12 @@  def check_not_nfs(path, name):
         return "The %s: %s can't be located on nfs.\n" % (name, path)
     return ""
 
+# Check that path isn't a broken symlink
+def check_symlink(lnk):
+    if os.path.islink(lnk) and not os.path.exists(lnk):
+       return False
+    return True
+
 def check_connectivity(d):
     # URI's to check can be set in the CONNECTIVITY_CHECK_URIS variable
     # using the same syntax as for SRC_URI. If the variable is not set
@@ -532,6 +538,8 @@  def check_sanity_sstate_dir_change(sstate_dir, data):
     # Check that SSTATE_DIR isn't on a filesystem with limited filename length (eg. eCryptFS)
     testmsg = ""
     if sstate_dir != "":
+        if not check_symlink(sstate_dir):
+            raise_sanity_error("SSTATE_DIR %s is a broken symlink." % sstate_dir, data)
         testmsg = check_create_long_filename(sstate_dir, "SSTATE_DIR")
         # If we don't have permissions to SSTATE_DIR, suggest the user set it as an SSTATE_MIRRORS
         try:
@@ -695,6 +703,8 @@  def check_sanity_everybuild(status, d):
         status.addresult("DL_DIR is not set. Your environment is misconfigured, check that DL_DIR is set, and if the directory exists, that it is writable. \n")
     if os.path.exists(dldir) and not os.access(dldir, os.W_OK):
         status.addresult("DL_DIR: %s exists but you do not appear to have write access to it. \n" % dldir)
+    if not check_symlink(dldir):
+        status.addresult("DL_DIR: %s is a broken symlink." % dldir)
 
     # Check that the MACHINE is valid, if it is set
     machinevalid = True
@@ -788,8 +798,19 @@  def check_sanity_everybuild(status, d):
                 bb.warn('Invalid protocol in %s: %s' % (mirror_var, mirror_entry))
                 continue
 
-            if mirror.startswith('file://') and not mirror.startswith('file:///'):
-                bb.warn('Invalid file url in %s: %s, must be absolute path (file:///)' % (mirror_var, mirror_entry))
+            if mirror.startswith('file://'):
+                if not mirror.startswith('file:///'):
+                    bb.warn('Invalid file url in %s: %s, must be absolute path (file:///)' % (mirror_var, mirror_entry))
+                import urlparse
+                if not check_symlink(urlparse.urlparse(mirror).path):
+                    raise_sanity_error("Mirror %s is a broken symlink." % mirror_entry, d)
+                # SSTATE_MIRROR ends with a /PATH string
+                if mirror.endswith('/PATH'):
+                    # remove /PATH$ from SSTATE_MIRROR to get a working
+                    # base directory path
+                    mirror_base = urlparse.urlparse(mirror[:-1*len('/PATH')]).path
+                    if not check_symlink(mirror_base):
+                        raise_sanity_error("State mirror %s is a broken symlink." % mirror_base, d)
 
     # Check that TMPDIR hasn't changed location since the last time we were run
     tmpdir = d.getVar('TMPDIR', True)

Comments

Mikko Rapeli Aug. 10, 2015, 8:18 a.m.
On Fri, Jul 31, 2015 at 12:00:15PM +0300, Mikko Rapeli wrote:
> This change makes broken symlinks stand out clearly instead of bitbake
> failing with odd error messages. Tested locally with broken symlink
> as SSTATE_DIR, DL_DIR and SSTATE_MIRROR.o currently oe-core isn't 

So currently patch testing and review queues are full.

Should I file bugzilla tickets with links to patches like this or
are the mailing list contributions tracked via patchwork or something?

If small changes like this are not getting merged, then I don't have confidence
in pushing bigger ones back upstream.

-Mikko

> Change-Id: I2e92702237ab3bdb897d0bdefcf33480aabbc288
> Signed-off-by: Mikko Rapeli <mikko.rapeli@bmw.de>
> ---
>  meta/classes/sanity.bbclass | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
> index 5be5efb..45ca992 100644
> --- a/meta/classes/sanity.bbclass
> +++ b/meta/classes/sanity.bbclass
> @@ -253,6 +253,12 @@ def check_not_nfs(path, name):
>          return "The %s: %s can't be located on nfs.\n" % (name, path)
>      return ""
>  
> +# Check that path isn't a broken symlink
> +def check_symlink(lnk):
> +    if os.path.islink(lnk) and not os.path.exists(lnk):
> +       return False
> +    return True
> +
>  def check_connectivity(d):
>      # URI's to check can be set in the CONNECTIVITY_CHECK_URIS variable
>      # using the same syntax as for SRC_URI. If the variable is not set
> @@ -532,6 +538,8 @@ def check_sanity_sstate_dir_change(sstate_dir, data):
>      # Check that SSTATE_DIR isn't on a filesystem with limited filename length (eg. eCryptFS)
>      testmsg = ""
>      if sstate_dir != "":
> +        if not check_symlink(sstate_dir):
> +            raise_sanity_error("SSTATE_DIR %s is a broken symlink." % sstate_dir, data)
>          testmsg = check_create_long_filename(sstate_dir, "SSTATE_DIR")
>          # If we don't have permissions to SSTATE_DIR, suggest the user set it as an SSTATE_MIRRORS
>          try:
> @@ -695,6 +703,8 @@ def check_sanity_everybuild(status, d):
>          status.addresult("DL_DIR is not set. Your environment is misconfigured, check that DL_DIR is set, and if the directory exists, that it is writable. \n")
>      if os.path.exists(dldir) and not os.access(dldir, os.W_OK):
>          status.addresult("DL_DIR: %s exists but you do not appear to have write access to it. \n" % dldir)
> +    if not check_symlink(dldir):
> +        status.addresult("DL_DIR: %s is a broken symlink." % dldir)
>  
>      # Check that the MACHINE is valid, if it is set
>      machinevalid = True
> @@ -788,8 +798,19 @@ def check_sanity_everybuild(status, d):
>                  bb.warn('Invalid protocol in %s: %s' % (mirror_var, mirror_entry))
>                  continue
>  
> -            if mirror.startswith('file://') and not mirror.startswith('file:///'):
> -                bb.warn('Invalid file url in %s: %s, must be absolute path (file:///)' % (mirror_var, mirror_entry))
> +            if mirror.startswith('file://'):
> +                if not mirror.startswith('file:///'):
> +                    bb.warn('Invalid file url in %s: %s, must be absolute path (file:///)' % (mirror_var, mirror_entry))
> +                import urlparse
> +                if not check_symlink(urlparse.urlparse(mirror).path):
> +                    raise_sanity_error("Mirror %s is a broken symlink." % mirror_entry, d)
> +                # SSTATE_MIRROR ends with a /PATH string
> +                if mirror.endswith('/PATH'):
> +                    # remove /PATH$ from SSTATE_MIRROR to get a working
> +                    # base directory path
> +                    mirror_base = urlparse.urlparse(mirror[:-1*len('/PATH')]).path
> +                    if not check_symlink(mirror_base):
> +                        raise_sanity_error("State mirror %s is a broken symlink." % mirror_base, d)
>  
>      # Check that TMPDIR hasn't changed location since the last time we were run
>      tmpdir = d.getVar('TMPDIR', True)
> -- 
> 2.4.6
>
mike.looijmans@topic.nl Aug. 10, 2015, 9:10 a.m.
?On 10-08-15 10:18, Mikko.Rapeli@bmw.de wrote:
> On Fri, Jul 31, 2015 at 12:00:15PM +0300, Mikko Rapeli wrote:
...
>> +# Check that path isn't a broken symlink
>> +def check_symlink(lnk):
>> +    if os.path.islink(lnk) and not os.path.exists(lnk):
>> +       return False
>> +    return True

- Bad coding style "if (x) return false".
- Naming a method "check..." suggests to me that it will raise an exception on 
failure.

alternatives:

def is_broken_symlink(lnk):
     return os.path.islink(lnk) and not os.path.exists(lnk)

def check_symlink(lnk, data):
     if os.path.islink(lnk) and not os.path.exists(lnk):
	raise_sanity_error("%s is a broken symlink." % lnk, data)


Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail
Mikko Rapeli Aug. 10, 2015, 11:48 a.m.
Hi,

On Mon, Aug 10, 2015 at 11:10:58AM +0200, Mike Looijmans wrote:
> On 10-08-15 10:18, Mikko.Rapeli@bmw.de wrote:
> >On Fri, Jul 31, 2015 at 12:00:15PM +0300, Mikko Rapeli wrote:
> ...
> >>+# Check that path isn't a broken symlink
> >>+def check_symlink(lnk):
> >>+    if os.path.islink(lnk) and not os.path.exists(lnk):
> >>+       return False
> >>+    return True
> 
> - Bad coding style "if (x) return false".
> - Naming a method "check..." suggests to me that it will raise an exception
> on failure.

Thanks. I tried to follow existing patterns from the functions there.

> alternatives:
> 
> def is_broken_symlink(lnk):
>     return os.path.islink(lnk) and not os.path.exists(lnk)
> 
> def check_symlink(lnk, data):
>     if os.path.islink(lnk) and not os.path.exists(lnk):
> 	raise_sanity_error("%s is a broken symlink." % lnk, data)

This feels better and reduces the log statements too. Hopefully the generic
error message is enough to figure out what is wrong. I'll test and send a
new version.

Regarding testing, I guess selftests are not generally used to test error
paths like this.

-Mikko
Ross Burton Aug. 10, 2015, 4:35 p.m.
On 10 August 2015 at 09:18, <Mikko.Rapeli@bmw.de> wrote:

> So currently patch testing and review queues are full.
>
> Should I file bugzilla tickets with links to patches like this or
> are the mailing list contributions tracked via patchwork or something?
>
> If small changes like this are not getting merged, then I don't have
> confidence
> in pushing bigger ones back upstream.
>

There's been slow acceptance of patches on master recently, mainly due to a
focus on making the autobuilder do green runs reliably.  To compound that,
I've just returned from a week off and Richard was travelling last week and
is still travelling this week.

That said, master-next is almost 50 commits ahead of master and will be
merged soon, and I've just queued this into my testing branch.  Stuff is
flowing, just slowly.

Ross
Mikko Rapeli Aug. 10, 2015, 5:02 p.m.
On Mon, Aug 10, 2015 at 05:35:27PM +0100, Burton, Ross wrote:
> On 10 August 2015 at 09:18, <Mikko.Rapeli@bmw.de> wrote:
> 
> > So currently patch testing and review queues are full.
> >
> > Should I file bugzilla tickets with links to patches like this or
> > are the mailing list contributions tracked via patchwork or something?
> >
> > If small changes like this are not getting merged, then I don't have
> > confidence
> > in pushing bigger ones back upstream.
> >
> 
> There's been slow acceptance of patches on master recently, mainly due to a
> focus on making the autobuilder do green runs reliably.  To compound that,
> I've just returned from a week off and Richard was travelling last week and
> is still travelling this week.
> 
> That said, master-next is almost 50 commits ahead of master and will be
> merged soon, and I've just queued this into my testing branch.  Stuff is
> flowing, just slowly.

Thanks for the update. I was just wondering if there's anything I could/should
do as a drive-by-contributor.

-Mikko