[PATCH 0/4 v3][PULL] "__depends/__base_depends" ordering, config hash improvement, and Hob fix

[PATCH 0/4 v3][PULL] "__depends/__base_depends" ordering, config hash improvement, and Hob fix

[Dongxiao Xu]

Hi Richard,

This is the third pull request for some Hob and Bitbake bug fixes, please help to review and pull.

This pull request contains commits that:
1) Retain order of "__depends" and "__base_depends".
2) Improve the config hash calculation.
3) Enlarge the upper value for image size spinner.
4) Set stop button insensitive before hide it

Changes from v2:
Add a new commit that set the stop button insensitive before hide it.

Thanks,
Dongxiao

The following changes since commit 98694c1dbc276cc151f393db67bfd43442da28ba:

  Hob: fixed a little view issue about package selection page (2012-04-16 12:56:15 +0100)

are available in the git repository at:
  git://git.pokylinux.org/poky-contrib dxu4/hob-bugfix
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=dxu4/hob-bugfix

Dongxiao Xu (4):
  bitbake: Retain order for __depends and __base_depends
  data_smart: Improve the calculation of config hash
  Hob: Enlarge the upper value of image size
  Hob: Set the "stop" button insensitive before hide it

 lib/bb/cache.py                      |    6 ++++--
 lib/bb/cooker.py                     |    6 ++++--
 lib/bb/data_smart.py                 |    7 ++++---
 lib/bb/parse/__init__.py             |   12 ++++++++----
 lib/bb/ui/crumbs/builddetailspage.py |    1 +
 lib/bb/ui/crumbs/hig.py              |    8 ++++----
 6 files changed, 25 insertions(+), 15 deletions(-)

-- 
1.7.4.1

[PATCH 1/4] bitbake: Retain order for __depends and __base_depends

[Dongxiao Xu]

Bitbake take seriously with variables order, therefore when setting
values to __depends and __base_depends, we need to retain its order.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 lib/bb/cache.py          |    6 ++++--
 lib/bb/cooker.py         |    6 ++++--
 lib/bb/parse/__init__.py |   12 ++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/lib/bb/cache.py b/lib/bb/cache.py
index 47e814b..a114582 100644
--- a/lib/bb/cache.py
+++ b/lib/bb/cache.py
@@ -391,12 +391,14 @@ class Cache(object):
         """Parse the specified filename, returning the recipe information"""
         infos = []
         datastores = cls.load_bbfile(filename, appends, configdata)
-        depends = set()
+        depends = []
         for variant, data in sorted(datastores.iteritems(),
                                     key=lambda i: i[0],
                                     reverse=True):
             virtualfn = cls.realfn2virtual(filename, variant)
-            depends |= (data.getVar("__depends", False) or set())
+            for dep in (data.getVar("__depends", False) or []):
+                if dep not in depends:
+                    depends.append(dep)
             if depends and not variant:
                 data.setVar("__depends", depends)
 
diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
index dea0aad..51341fa 100644
--- a/lib/bb/cooker.py
+++ b/lib/bb/cooker.py
@@ -680,8 +680,10 @@ class BBCooker:
         # Generate a list of parsed configuration files by searching the files
         # listed in the __depends and __base_depends variables with a .conf suffix.
         conffiles = []
-        dep_files = self.configuration.data.getVar('__depends') or set()
-        dep_files.union(self.configuration.data.getVar('__base_depends') or set())
+        dep_files = self.configuration.data.getVar('__depends') or []
+        for dep in (self.configuration.data.getVar('__base_depends') or []):
+            if dep not in dep_files:
+                dep_files.append(dep)
 
         for f in dep_files:
             if f[0].endswith(".conf"):
diff --git a/lib/bb/parse/__init__.py b/lib/bb/parse/__init__.py
index 7b9c47e..f223ff9 100644
--- a/lib/bb/parse/__init__.py
+++ b/lib/bb/parse/__init__.py
@@ -73,8 +73,10 @@ def update_mtime(f):
 def mark_dependency(d, f):
     if f.startswith('./'):
         f = "%s/%s" % (os.getcwd(), f[2:])
-    deps = d.getVar('__depends') or set()
-    deps.update([(f, cached_mtime(f))])
+    deps = d.getVar('__depends') or []
+    t = cached_mtime(f)
+    if (f, t) not in deps:
+        deps.append((f, t))
     d.setVar('__depends', deps)
 
 def supports(fn, data):
@@ -134,8 +136,10 @@ def vars_from_file(mypkg, d):
 def get_file_depends(d):
     '''Return the dependent files'''
     dep_files = []
-    depends = d.getVar('__depends', True) or set()
-    depends = depends.union(d.getVar('__base_depends', True) or set())
+    depends = d.getVar('__depends', True) or []
+    for dep in (d.getVar('__base_depends', True) or []):
+        if dep not in depends:
+            depends.append(dep)
     for (fn, _) in depends:
         dep_files.append(os.path.abspath(fn))
     return " ".join(dep_files)
-- 
1.7.4.1

Re: [PATCH 1/4] bitbake: Retain order for __depends and __base_depends

[Richard Purdie]

Hi Dongxiao,

I've spent a while thinking about this patch and its implications. We
have several potential correctness issues here. I finally realised there
is a correctness issue for get_file_depends() with the ordering which
means I can't merge it :(. I'm going to give all the feedback I came up
with whilst considering it as there are other details we need to get
right.

The issue is that we need consider what we're using this variable for
and how. We need the data to be meaningful, useful and correct. We're
trying to use the data for multiple things and it needs to be correct in
each case.

On Tue, 2012-04-17 at 16:21 +0800, Dongxiao Xu wrote:
> Bitbake take seriously with variables order, therefore when setting
> values to __depends and __base_depends, we need to retain its order.
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> ---
>  lib/bb/cache.py          |    6 ++++--
>  lib/bb/cooker.py         |    6 ++++--
>  lib/bb/parse/__init__.py |   12 ++++++++----
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/bb/cache.py b/lib/bb/cache.py
> index 47e814b..a114582 100644
> --- a/lib/bb/cache.py
> +++ b/lib/bb/cache.py
> @@ -391,12 +391,14 @@ class Cache(object):
>          """Parse the specified filename, returning the recipe information"""
>          infos = []
>          datastores = cls.load_bbfile(filename, appends, configdata)
> -        depends = set()
> +        depends = []
>          for variant, data in sorted(datastores.iteritems(),
>                                      key=lambda i: i[0],
>                                      reverse=True):
>              virtualfn = cls.realfn2virtual(filename, variant)
> -            depends |= (data.getVar("__depends", False) or set())
> +            for dep in (data.getVar("__depends", False) or []):
> +                if dep not in depends:
> +                    depends.append(dep)
>              if depends and not variant:
>                  data.setVar("__depends", depends)

This one is the hardest to get right. We really need to:

* Save off the original __depends
* Check the returned __depends and compute what was added compared to
the original __depends value for each variant, then add only these
additions to the base __depends value.


> diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
> index dea0aad..51341fa 100644
> --- a/lib/bb/cooker.py
> +++ b/lib/bb/cooker.py
> @@ -680,8 +680,10 @@ class BBCooker:
>          # Generate a list of parsed configuration files by searching the files
>          # listed in the __depends and __base_depends variables with a .conf suffix.
>          conffiles = []
> -        dep_files = self.configuration.data.getVar('__depends') or set()
> -        dep_files.union(self.configuration.data.getVar('__base_depends') or set())
> +        dep_files = self.configuration.data.getVar('__depends') or []
> +        for dep in (self.configuration.data.getVar('__base_depends') or []):
> +            if dep not in dep_files:
> +                dep_files.append(dep)

Here we may as well do:

    dep_files = set(self.configuration.data.getVar('__depends') or [])
    dep_files.update(self.configuration.data.getVar('__base_depends') or [])

since order and duplicates don't matter.

>          for f in dep_files:
>              if f[0].endswith(".conf"):
> diff --git a/lib/bb/parse/__init__.py b/lib/bb/parse/__init__.py
> index 7b9c47e..f223ff9 100644
> --- a/lib/bb/parse/__init__.py
> +++ b/lib/bb/parse/__init__.py
> @@ -73,8 +73,10 @@ def update_mtime(f):
>  def mark_dependency(d, f):
>      if f.startswith('./'):
>          f = "%s/%s" % (os.getcwd(), f[2:])
> -    deps = d.getVar('__depends') or set()
> -    deps.update([(f, cached_mtime(f))])
> +    deps = d.getVar('__depends') or []
> +    t = cached_mtime(f)
> +    if (f, t) not in deps:
> +        deps.append((f, t))
>      d.setVar('__depends', deps)

I think this should be unconditional even if it introduces duplicates so
that we can know if the file was included more than once:

    deps = d.getVar('__depends') or []
    t = cached_mtime(f)
    deps.append((f, t))
    d.setVar('__depends', deps)

>  def supports(fn, data):
> @@ -134,8 +136,10 @@ def vars_from_file(mypkg, d):
>  def get_file_depends(d):
>      '''Return the dependent files'''
>      dep_files = []
> -    depends = d.getVar('__depends', True) or set()
> -    depends = depends.union(d.getVar('__base_depends', True) or set())
> +    depends = d.getVar('__depends', True) or []
> +    for dep in (d.getVar('__base_depends', True) or []):
> +        if dep not in depends:
> +            depends.append(dep)
>      for (fn, _) in depends:
>          dep_files.append(os.path.abspath(fn))
>      return " ".join(dep_files)

This is the one that really worried me. __base_depends needs to be
listed before __depends as that was the order used. Again, we shouldn't
be removing duplicates so:

    depends = d.getVar('__base_depends', True) or []
    for dep in (d.getVar('__depends', True) or []):
        depends.append(dep)

Also, in cache.py:cacheValidUpdate(), we might as well do something
like:

-  for f, old_mtime in depends:
+  for f, old_mtime in set(depends):

although this is a micro optimisation. We could do that when the value
is placed in the cache I guess and reduce cache size too.

Cheers,

Richard

Re: [PATCH 1/4] bitbake: Retain order for __depends and __base_depends

[Lu, Lianhao]

Richard Purdie wrote on 2012-04-17:
> Hi Dongxiao,
> 
> I've spent a while thinking about this patch and its implications. We
> have several potential correctness issues here. I finally realised there
> is a correctness issue for get_file_depends() with the ordering which

Currently the only user of get_file_depends() is the variable BBINCLUDED 
which is used by eclipse plugin. I'm not sure whether other uses of __(base_)depends
care about the order, but eclipse plugin doesn't care about the order very much,
as long as the patch doesn't change the BBINCLUDED content.

> means I can't merge it :(. I'm going to give all the feedback I came up
> with whilst considering it as there are other details we need to get
> right.
> 
> The issue is that we need consider what we're using this variable for
> and how. We need the data to be meaningful, useful and correct. We're
> trying to use the data for multiple things and it needs to be correct in
> each case.
> 
> On Tue, 2012-04-17 at 16:21 +0800, Dongxiao Xu wrote:
>> Bitbake take seriously with variables order, therefore when setting
>> values to __depends and __base_depends, we need to retain its order.
>> 
>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>> ---
>>  lib/bb/cache.py          |    6 ++++--
>>  lib/bb/cooker.py         |    6 ++++--
>>  lib/bb/parse/__init__.py |   12 ++++++++----
>>  3 files changed, 16 insertions(+), 8 deletions(-)
>> diff --git a/lib/bb/cache.py b/lib/bb/cache.py
>> index 47e814b..a114582 100644
>> --- a/lib/bb/cache.py
>> +++ b/lib/bb/cache.py
>> @@ -391,12 +391,14 @@ class Cache(object):
>>          """Parse the specified filename, returning the recipe information"""
>>          infos = []
>>          datastores = cls.load_bbfile(filename, appends, configdata)
>> -        depends = set()
>> +        depends = []
>>          for variant, data in sorted(datastores.iteritems(),
>>                                      key=lambda i: i[0],
>>                                      reverse=True):
>>              virtualfn = cls.realfn2virtual(filename, variant)
>> -            depends |= (data.getVar("__depends", False) or set())
>> +            for dep in (data.getVar("__depends", False) or []):
>> +                if dep not in depends:
>> +                    depends.append(dep)
>>              if depends and not variant:
>>                  data.setVar("__depends", depends)
> 
> This one is the hardest to get right. We really need to:
> 
> * Save off the original __depends
> * Check the returned __depends and compute what was added compared to
> the original __depends value for each variant, then add only these
> additions to the base __depends value.
> 
> 
>> diff --git a/lib/bb/cooker.py b/lib/bb/cooker.py
>> index dea0aad..51341fa 100644
>> --- a/lib/bb/cooker.py
>> +++ b/lib/bb/cooker.py
>> @@ -680,8 +680,10 @@ class BBCooker:
>>          # Generate a list of parsed configuration files by searching the files
>>          # listed in the __depends and __base_depends variables with a .conf suffix.
>>          conffiles = []
>> -        dep_files = self.configuration.data.getVar('__depends') or set()
>> -        dep_files.union(self.configuration.data.getVar('__base_depends') or set())
>> +        dep_files = self.configuration.data.getVar('__depends') or []
>> +        for dep in (self.configuration.data.getVar('__base_depends') or []):
>> +            if dep not in dep_files:
>> +                dep_files.append(dep)
> 
> Here we may as well do:
> 
>     dep_files = set(self.configuration.data.getVar('__depends') or [])
>     dep_files.update(self.configuration.data.getVar('__base_depends') or [])
> since order and duplicates don't matter.
> 
>>          for f in dep_files:
>>              if f[0].endswith(".conf"):
>> diff --git a/lib/bb/parse/__init__.py b/lib/bb/parse/__init__.py
>> index 7b9c47e..f223ff9 100644
>> --- a/lib/bb/parse/__init__.py
>> +++ b/lib/bb/parse/__init__.py
>> @@ -73,8 +73,10 @@ def update_mtime(f):
>>  def mark_dependency(d, f):
>>      if f.startswith('./'):
>>          f = "%s/%s" % (os.getcwd(), f[2:])
>> -    deps = d.getVar('__depends') or set()
>> -    deps.update([(f, cached_mtime(f))])
>> +    deps = d.getVar('__depends') or []
>> +    t = cached_mtime(f)
>> +    if (f, t) not in deps:
>> +        deps.append((f, t))
>>      d.setVar('__depends', deps)
> 
> I think this should be unconditional even if it introduces duplicates so
> that we can know if the file was included more than once:
> 
>     deps = d.getVar('__depends') or []
>     t = cached_mtime(f)
>     deps.append((f, t))
>     d.setVar('__depends', deps)
>>  def supports(fn, data): @@ -134,8 +136,10 @@ def vars_from_file(mypkg,
>>  d): def get_file_depends(d):
>>      '''Return the dependent files'''
>>      dep_files = []
>> -    depends = d.getVar('__depends', True) or set()
>> -    depends = depends.union(d.getVar('__base_depends', True) or set())
>> +    depends = d.getVar('__depends', True) or []
>> +    for dep in (d.getVar('__base_depends', True) or []):
>> +        if dep not in depends:
>> +            depends.append(dep)
>>      for (fn, _) in depends:
>>          dep_files.append(os.path.abspath(fn))
>>      return " ".join(dep_files)
> 
> This is the one that really worried me. __base_depends needs to be
> listed before __depends as that was the order used. Again, we shouldn't
> be removing duplicates so:
> 
>     depends = d.getVar('__base_depends', True) or []
>     for dep in (d.getVar('__depends', True) or []):
>         depends.append(dep)
> Also, in cache.py:cacheValidUpdate(), we might as well do something like:
> 
> -  for f, old_mtime in depends:
> +  for f, old_mtime in set(depends):
> 
> although this is a micro optimisation. We could do that when the value
> is placed in the cache I guess and reduce cache size too.
> 
> Cheers,
> 
> Richard
> 
> 
> 
> _______________________________________________
> bitbake-devel mailing list
> bitbake-devel@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/bitbake-devel

Best Regards,
Lianhao

[PATCH 2/4] data_smart: Improve the calculation of config hash

[Dongxiao Xu]

For config hash, we put the keys in structure of "set()", which is not
order sensitive. Therefore when calculating the md5 value for config
hash, we need to identify the order of the keys.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 lib/bb/data_smart.py |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index 2c200db..27fb7d9 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -462,13 +462,14 @@ class DataSmart(MutableMapping):
         self.delVar(var)
 
     def get_hash(self):
-        data = ""
+        data = {}
         config_whitelist = set((self.getVar("BB_HASHCONFIG_WHITELIST", True) or "").split())
         keys = set(key for key in iter(self) if not key.startswith("__"))
         for key in keys:
             if key in config_whitelist:
                 continue
             value = self.getVar(key, False) or ""
-            data = data + key + ': ' + str(value) + '\n'
+            data.update({key:value})
 
-        return hashlib.md5(data).hexdigest()
+        data_str = str([(k, data[k]) for k in sorted(data.keys())])
+        return hashlib.md5(data_str).hexdigest()
-- 
1.7.4.1

[PATCH 3/4] Hob: Enlarge the upper value of image size

[Dongxiao Xu]

Originally the upper value for image size and extra size is 1024M, which
is relatively small. Enlarge it to 64GB.

Besides, fix tooltip for toolchain build.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 lib/bb/ui/crumbs/hig.py |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/bb/ui/crumbs/hig.py b/lib/bb/ui/crumbs/hig.py
index a85b478..93a4fbc 100644
--- a/lib/bb/ui/crumbs/hig.py
+++ b/lib/bb/ui/crumbs/hig.py
@@ -423,15 +423,15 @@ class AdvancedSettingDialog (CrumbsDialog):
         advanced_vbox.pack_start(sub_vbox, expand=False, fill=False)
         label = self.gen_label_widget("<span weight=\"bold\">Image rootfs size: (MB)</span>")
         tooltip = "Sets the basic size of your target image.\nThis is the basic size of your target image unless your selected package size exceeds this value or you select \'Image Extra Size\'."
-        rootfs_size_widget, self.rootfs_size_spinner = self.gen_spinner_widget(int(self.configuration.image_rootfs_size*1.0/1024), 0, 1024, tooltip)
+        rootfs_size_widget, self.rootfs_size_spinner = self.gen_spinner_widget(int(self.configuration.image_rootfs_size*1.0/1024), 0, 65536, tooltip)
         sub_vbox.pack_start(label, expand=False, fill=False)
         sub_vbox.pack_start(rootfs_size_widget, expand=False, fill=False)
 
         sub_vbox = gtk.VBox(False, 6)
         advanced_vbox.pack_start(sub_vbox, expand=False, fill=False)
         label = self.gen_label_widget("<span weight=\"bold\">Image extra size: (MB)</span>")
-        tooltip = "Sets the extra free space of your target image.\nBy default, the system reserves 30% of your image size as free space. If your image contains zypper, it brings in 50MB more space. The maximum free space is 1024MB."
-        extra_size_widget, self.extra_size_spinner = self.gen_spinner_widget(int(self.configuration.image_extra_size*1.0/1024), 0, 1024, tooltip)
+        tooltip = "Sets the extra free space of your target image.\nBy default, the system reserves 30% of your image size as free space. If your image contains zypper, it brings in 50MB more space. The maximum free space is 64GB."
+        extra_size_widget, self.extra_size_spinner = self.gen_spinner_widget(int(self.configuration.image_extra_size*1.0/1024), 0, 65536, tooltip)
         sub_vbox.pack_start(label, expand=False, fill=False)
         sub_vbox.pack_start(extra_size_widget, expand=False, fill=False)
 
@@ -450,7 +450,7 @@ class AdvancedSettingDialog (CrumbsDialog):
         self.toolchain_checkbox.set_active(self.configuration.toolchain_build)
         sub_hbox.pack_start(self.toolchain_checkbox, expand=False, fill=False)
 
-        tooltip = "Selects the Host platform for which you want to run the toolchain"
+        tooltip = "Selects the host platform for which you want to run the toolchain"
         sdk_machine_widget, self.sdk_machine_combo = self.gen_combo_widget(self.configuration.curr_sdk_machine, self.all_sdk_machines, tooltip)
         sub_hbox.pack_start(sdk_machine_widget, expand=False, fill=False)
 
-- 
1.7.4.1

[PATCH 4/4] Hob: Set the "stop" button insensitive before hide it

[Dongxiao Xu]

If user stops a build, we need to firstly set the button insensitive and
then hide it. This ensures the button's init status is "insensitive" in
next build.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 lib/bb/ui/crumbs/builddetailspage.py |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/bb/ui/crumbs/builddetailspage.py b/lib/bb/ui/crumbs/builddetailspage.py
index 6d8b509..51e6a4a 100755
--- a/lib/bb/ui/crumbs/builddetailspage.py
+++ b/lib/bb/ui/crumbs/builddetailspage.py
@@ -238,6 +238,7 @@ class BuildDetailsPage (HobPage):
         self.builder.stop_build()
 
     def hide_stop_button(self):
+        self.stop_button.set_sensitive(False)
         self.stop_button.hide()
 
     def scroll_to_present_row(self, model, path, iter, v_adj, treeview):
-- 
1.7.4.1