Re: [PATCH 3/6] classes/dpkg: Split install for cache

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi,

On 08/29/2017 09:18 AM, Alexander Smirnov wrote:
> 
> 
> On 08/28/2017 11:00 AM, Claudius Heine wrote:
>> Hi,
>>
>> On 08/27/2017 05:13 PM, Alexander Smirnov wrote:
>>> Split install function into two parts:
>>>   - install_to_cache: if caching is enabled
>>>   - install_to_deploy: if caching is disabled
>>>
>>> This patch brings flexibility to the implementation and makes it 
>>> possible
>>> to move all the caching implementation code to dedicated class. The 
>>> magic
>>> behavior depending on the value of DEBCACHE_ENABLED is now 
>>> transparent in
>>> the bitbake pipeline (can be inspected via "bitbake -g").
>>>
>>> Signed-off-by: Alexander Smirnov <asmirnov@ilbers.de>
>>> ---
>>>   meta/classes/dpkg.bbclass  | 66 
>>> ++++++++++++++++++++++++++--------------------
>>>   meta/classes/image.bbclass |  2 +-
>>>   2 files changed, 38 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
>>> index b1e201d..118ba2f 100644
>>> --- a/meta/classes/dpkg.bbclass
>>> +++ b/meta/classes/dpkg.bbclass
>>> @@ -56,46 +56,53 @@ do_build() {
>>>   }
>>>   # Install package to dedicated deploy directory
>>> -do_binary_deb_install() {
>>> +do_install_to_deploy() {
>>> +    # Deb caching is disabled, simply copy all binary packages to 
>>> the deploy
>>> +    # directory
>>> +    install -m 644 "${BUILDROOT}"/*.deb "${DEPLOY_DIR_DEB}/"
>>> +}
>>> +
>>> +addtask install_to_deploy after do_build
>>> +do_install_to_deploy[dirs] = "${DEPLOY_DIR_DEB}"
>>> +do_install_to_deploy[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>> +do_install_to_deploy[noexec] = "1"
>>> +
>>> +# Install package to dedicated apt repo
>>> +do_install_to_cache() {
>>>       readonly DIR_CACHE="${DEBCACHEDIR}/${DISTRO}"
>>>       readonly DIR_DB="${DEBDBDIR}/${DISTRO}"
>>> -    if [ "${DEBCACHE_ENABLED}" != "0" ]; then
>>> -        # If `bitbake` is running for the first time, the cache 
>>> doesn't exist
>>> -        # yet and needs to be configured using a `distributions` file.
>>> -        # A template stored in the layer directory is pre-processed to
>>> -        # generate the configuration file, which is then placed in the
>>> -        # appropriate directory.
>>> -        if [ ! -e "${DIR_CACHE}/conf/distributions" ]; then
>>> -            mkdir -p "${DIR_CACHE}/conf"
>>> -            sed -e "s#{DISTRO_NAME}#${DEBDISTRONAME}#g" \
>>> -                "${DEBFILESDIR}/distributions.in" \
>>> -                > "${DIR_CACHE}/conf/distributions"
>>> -        fi
>>> -
>>> -        # Add binary and source packages to the deb cache
>>> -        # If the cache doesn't exist yet, it will be created using the
>>> -        # `distributions` file generated above.
>>> -        ls -1 "${BUILDROOT}"/*.deb "${BUILDROOT}"/*.dsc | while read 
>>> -r p; do
>>> -            reprepro --waitforlock 3 -b "${DIR_CACHE}" --dbdir 
>>> "${DIR_DB}" \
>>> -                -C main "include${p##*.}" "${DEBDISTRONAME}" "${p}"
>>> -        done
>>> -    else
>>> -        # Deb caching is disabled, simply copy all binary packages 
>>> to the
>>> -        # deploy directory
>>> -        mkdir -p "${DEPLOY_DIR_DEB}"
>>> -        install -m 644 "${BUILDROOT}"/*.deb "${DEPLOY_DIR_DEB}/"
>>> +    # If `bitbake` is running for the first time, the cache doesn't 
>>> exist
>>> +    # yet and needs to be configured using a `distributions` file.
>>> +    # A template stored in the layer directory is pre-processed to
>>> +    # generate the configuration file, which is then placed in the
>>> +    # appropriate directory.
>>> +    if [ ! -e "${DIR_CACHE}/conf/distributions" ]; then
>>> +        mkdir -p "${DIR_CACHE}/conf"
>>> +        sed -e "s#{DISTRO_NAME}#${DEBDISTRONAME}#g" \
>>> +            "${DEBFILESDIR}/distributions.in" \
>>> +            > "${DIR_CACHE}/conf/distributions"
>>>       fi
>>> +
>>> +    # Add binary and source packages to the deb cache
>>> +    # If the cache doesn't exist yet, it will be created using the
>>> +    # `distributions` file generated above.
>>> +    ls -1 "${BUILDROOT}"/*.deb "${BUILDROOT}"/*.dsc | while read -r 
>>> p; do
>>> +        reprepro --waitforlock 3 -b "${DIR_CACHE}" --dbdir 
>>> "${DIR_DB}" \
>>> +            -C main "include${p##*.}" "${DEBDISTRONAME}" "${p}"
>>> +    done
>>>   }
>>> -addtask binary_deb_install after do_build
>>> -do_binary_deb_install[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>> +addtask install_to_cache after do_build
>>
>> What was the reason why we put stuff after the 'build' task? In OE the 
>> 'build' task is the default task triggering everything necessary for a 
>> recipe. In Isar that changed. Why?
> 
> Actually AFAIK do_build is not OE tasks, it's default bitbake task. 
> Install is after do_build, because in Isar do_build is used to build the 
> packages, so it's not empty.

You are right, its a bitbake task. Since I have much more experience 
with OE than with just bitbake or isar, I referenced my question to the 
experience I had using OE. I cannot remember seeing tasks that are added 
after the do_build task in OE and I still find that very strange. AFAIK 
the 'build' task in bitbake and OE has a similar meaning to the 'all' 
target in make files. The idea to add statements to the 'all' target or 
letting other targets depend on the 'all' target sounds very odd to me. 
And that is what is done here in isar. Just very strange and unusual for 
my eyes and I don't see the advantage of this.

I would like to hear the reason for this decision. Why have you added 
statements to the 'build' task and why are other tasks depend on it?

>>
>>> +do_install_to_cache[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>> +do_install_to_cache[noexec] = "1"
>>>   # Deb caching lambda run during the parsing phase that checks 
>>> whether the
>>>   # current package has to be rebuilt, or taken from the cache
>>>   python __anonymous () {
>>>       if d.getVar("DEBCACHE_ENABLED", True) == "0":
>>> -        # Deb caching is disabled, do nothing
>>> +        # Deb caching is disabled
>>> +        d.delVarFlag('do_install_to_deploy', 'noexec')
>>>           return True
>>>       PN = d.getVar("PN", True)
>>> @@ -108,6 +115,7 @@ python __anonymous () {
>>>       path_cache = os.path.join(DEBCACHEDIR, DISTRO)
>>>       path_databases = os.path.join(DEBDBDIR, DISTRO)
>>>       path_distributions = os.path.join(path_cache, "conf", 
>>> "distributions")
>>> +    d.delVarFlag('do_install_to_cache', 'noexec')
>>>       # The distributions file is needed by `reprepro` to know what 
>>> types
>>>       # of packages are supported, what the distribution name is, etc.
>>> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>>> index c2ff453..6b1b5eb 100644
>>> --- a/meta/classes/image.bbclass
>>> +++ b/meta/classes/image.bbclass
>>> @@ -46,4 +46,4 @@ do_populate() {
>>>   }
>>>   addtask populate before do_build
>>> -do_populate[deptask] = "do_install"
>>> +do_populate[deptask] = "do_install_to_cache do_install_to_deploy"
>>
>> I would rather still have a, maybe empty, 'do_install' step that just 
>> depends on the 'do_install_to_cache' and 'do_install_to_deploy' tasks. 
>> I don't see why ever small change in one class should change the 
>> complete pipeline for every recipe.
> 
> It's a bit difficult question. In general meaning when building 
> something from sources, the install means a storing of build artifacts 
> to some specific location. So in this case, we  just copy binary deb 
> package to repo, and there is nothing common with installation of 
> artifacts.
> 
> Also Isar has in roadmap cross-compilation, so the pipeline will 
> probably look like:
> 
> fetch - unpack - compile - install - install_to_cache
> 
> so in this case the install task probably should go before 
> install_to_cache.
> 
> So probably it makes sense to avoid _install_ in this task, for example 
> use do_populate_deb_cache, what do you think?

Something like: 'populate_repo' sounds much better for this case. I 
would try to have more abstract names at first and implement them with 
more specific names like 'populate_repo_deb'. In OE there already is a 
'package' and additional sub tasks like 'package_write_deb'.

So a general pipeline for normal recipes like this:

fetch, unpack, configure, compile, install, package, populate_repo

Of course image recipes would have a slightly changed pipeline.

I think the discussion about the build pipeline in isar is very 
important. Maybe we should make an extra thread about this.

Cheers,
Claudius

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de