Fixing bitbake --revisions-changed

* Fixing bitbake --revisions-changed
@ 2018-10-04 15:10 chris.laplante
  2018-10-24 14:58 ` chris.laplante
  2019-02-21 21:16 ` Richard Purdie
  0 siblings, 2 replies; 3+ messages in thread
From: chris.laplante @ 2018-10-04 15:10 UTC (permalink / raw)
  To: bitbake-devel

Hi all,

A while back I wanted to use the --revisions-changed flag of bitbake but found that it was unfortunately broken. In the next week or so I hope to submit a patchset to fix it. There are 3 issues preventing it from working, which I'll detail below. I am not sure how to fix the third issue, so that's why I'm starting this conversation.

Please note that to reproduce these results, vanilla Poky isn't enough - none of its recipes use SRCREV = "${AUTOREV}". So you'll either need to add a recipe (or layer with a recipe) that uses it.

1. Bad call to bb.fetch.fetcher_compare_revisions

The first issue you'll encounter if you try the flag is this exception:

    ERROR: Command execution failed: Traceback (most recent call last):
      File "/home/laplante/poky-master/poky/bitbake/lib/bb/command.py", line 116, in runAsyncCommand
        commandmethod(self.cmds_async, self, options)
      File "/home/laplante/poky-master/poky/bitbake/lib/bb/command.py", line 723, in compareRevisions
        if bb.fetch.fetcher_compare_revisions(command.cooker.data):
    TypeError: fetcher_compare_revisions() takes 0 positional arguments but 1 was given

BitBake rev 81bc1f20662c39ee8db1da45b1e8c7eb64abacf3 (2016) mistakenly dropped the 'd' parameter of fetcher_compare_revisions. The sole usage of the method (in command.py) still correctly passes it. So the fix here is easy - just add back the 'd' to fetcher_compare_revisions.

2. Incorrect handling of bb.command.CommandExit in knotty.py

After you've fixed (1), and if upstream changes are "detected", then compareRevisions (command.py) will call command.finishAsyncCommand(code=1). That method will fire bb.command.CommandExit. Assuming you are using knotty, your bitbake command will hang forever. This is because the knotty CommandExit handler does not set "main.shutdown" (unlike the CommandFinish and CommandCompleted handlers). 

The fix here is simple - the CommandExit handler should set "main.shutdown = 2" to break out of the loop and return its return code. This is in line with how the ncurses UI works (it exits the loop for all three flavors of events).

3. bb.fetch2.saved_headrevs is always empty inside fetcher_compare_revisions, because fetcher_init is called twice.

Now we get to the part we're I'm unsure of how to proceed. In (2), "detected" is quoted because as we shall see, fetcher_compare_revisions will always think it detects changes.

I will assume BB_SRCREV_POLICY is set to its default "clear". fetcher_compare_revisions boils down to comparing these two sets of revisions:

    data = bb.persist_data.persist('BB_URI_HEADREVS', d).items()
    data2 = bb.fetch2.saved_headrevs

If you instrument the code with some extra logging, you'll find that data2 (and therefore bb.fetch2.saved_headrevs) is always empty. The problem is that fetcher_init gets called twice. I'll reproduce just the first part of the method:

    def fetcher_init(d):
        # ...
        srcrev_policy = d.getVar('BB_SRCREV_POLICY') or "clear"
        if srcrev_policy == "cache":
            logger.debug(1, "Keeping SRCREV cache due to cache policy of: %s", srcrev_policy)
        elif srcrev_policy == "clear":
            logger.debug(1, "Clearing SRCREV cache due to cache policy of: %s", srcrev_policy)
            revs = bb.persist_data.persist('BB_URI_HEADREVS', d)
            try: 
                bb.fetch2.saved_headrevs = revs.items()
            except:
                pass 
            revs.clear()

Again, let's assume BB_SRCREV_POLICY is "clear". The first time the method is called (during server startup), bb.fetch2.saved_headrevs is correctly set and BB_URI_HEADREVS is rightfully cleared.

Unfortunately, fetcher_init gets called again later by runCommand (command.py). The sequence of method calls is command.py:runCommand => command.py:updateConfig => cooker.py:updateConfigOpts => cooker.py:reset => cooker.py:initConfigurationData => cookerdata.py:parseBaseConfiguration => bb.fetch.fetcher_init. 

In this second invocation, bb.fetch2.saved_headrevs is again set based on BB_URI_HEADREVS. But the first call to fetcher_init cleared BB_URI_HEADREVS... thus bb.fetch2.saved_headrevs ends up empty.

----

I'd like to hear some feedback on how to fix (3). The "easy" solution, I think, is to do something like this to ensure saved_headrevs doesn't get overwritten:

    if not bb.fetch2.saved_headrevs:
        bb.fetch2.saved_headrevs = revs.items()

fetcher_compare_revisions is the only thing that uses saved_headrevs, so this shouldn't break anything. Alternatively, is it better to change it so fetcher_init doesn't get called multiple times? Perhaps it should be split into a fetcher_init and fetcher_reset? 

(Another side question I have is whether we should/could modify fetcher_compare_revisions to work even when BB_SRCREV_POLICY is set to 'cache'. That functionality would seem very useful to me. That's probably a topic for another discussion thread though.)

Looking forward to feedback.

Thanks,
Chris

^ permalink raw reply	[flat|nested] 3+ messages in thread