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

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

[Mikko Rapeli]

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(-)

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

Re: [PATCH] sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

[Mikko.Rapeli]

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
> 

Re: [PATCH] sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

[Mike Looijmans]

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

Re: [PATCH] sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

[Mikko.Rapeli]

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

[PATCH v2] sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

[Mikko Rapeli]

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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

v2: Use raise_sanity_error() as suggested by Mike Looijmans
    <mike.looijmans@topic.nl>, move sstate cache symlink check to be
    executed on every bitbake run and not just when sanity versions
    changed.

diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
index 6ad620b..ef90fc8 100644
--- a/meta/classes/sanity.bbclass
+++ b/meta/classes/sanity.bbclass
@@ -259,6 +259,11 @@ 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, data):
+    if os.path.islink(lnk) and not os.path.exists(lnk):
+        raise_sanity_error("%s is a broken symlink." % lnk, data)
+
 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
@@ -718,6 +723,7 @@ 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)
+    check_symlink(dldir, d)
 
     # Check that the MACHINE is valid, if it is set
     machinevalid = True
@@ -811,8 +817,17 @@ 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
+                check_symlink(urlparse.urlparse(mirror).path, 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
+                    check_symlink(mirror_base, d)
 
     # Check that TMPDIR hasn't changed location since the last time we were run
     tmpdir = d.getVar('TMPDIR', True)
@@ -860,6 +875,8 @@ def check_sanity(sanity_data):
     tmpdir = sanity_data.getVar('TMPDIR', True)
     sstate_dir = sanity_data.getVar('SSTATE_DIR', True)
 
+    check_symlink(sstate_dir, sanity_data)
+
     # Check saved sanity info
     last_sanity_version = 0
     last_tmpdir = ""
-- 
2.4.6

Re: [PATCH] sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 1275 bytes --]

Re: [PATCH] sanity.bbclass: check SSTATE_DIR, DL_DIR and *MIRROR for broken symlinks

[Mikko.Rapeli]

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