+waihong

Tom, do you know any one is in charge of this metadata.json file? Please help us to redirect this bug. Thanks!

The firmware versions come from the firmware updater in the OS image, not the new firmware, check chromite/cbuildbot/commands.py.

def GetFirmwareVersions(buildroot, board):
  """Extract version information from the firmware updater, if one exists.

  Args:
    buildroot: The buildroot of the current build.
    board: The board the firmware is for.

  Returns:
    (main fw version, ec fw version)
    Each element will either be set to the string output by the firmware
    updater shellball, or None if there is no firmware updater.
  """
  updater = os.path.join(buildroot, constants.DEFAULT_CHROOT_DIR,
                         cros_build_lib.GetSysroot(board).lstrip(os.path.sep),
                         'usr', 'sbin', 'chromeos-firmwareupdate')
  if not os.path.isfile(updater):
    return FirmwareVersions(None, None)
  updater = path_util.ToChrootPath(updater)

  result = cros_build_lib.RunCommand([updater, '-V'], enter_chroot=True,
                                     capture_output=True, log_output=True,
                                     cwd=buildroot)
  main = re.search(r'BIOS version:\s*(?P<version>.*)', result.output)
  ec = re.search(r'EC version:\s*(?P<version>.*)', result.output)
  return (main.group('version') if main else None,
          ec.group('version') if ec else None)

Looks like for the firmware branch, the firmware and EC version should be extracted elsewhere other then the json file.

The Firmware version  should always match the branch, i am not sure what is an easy way to extract the EC version.

That's possible, but this has always worked before -- not sure why this is no longer working on firmware branches or what is special about chell in this situation.  This code was originally written by Gabe on the infra team, cc'ed Aviv as well.

Who uses this metadata.json?

It can be correct or wrong, depending on what these firmware versions are used. If it means the versions of firmware updater, it is correct. If it means the versions of new-builded firmware from source, it is wrong.

This is used by GoldenEye (go/goldeneye) and the versions are intended to be the actual FW versions, which are displayed along other build metadata and also recently started to be used for FW qualification purposes.

Is there an easy way to fix this? I haven't tested myself, but is it working the same way in all branches/builders?

I think the fix is easy, changing the GetFirmwareVersions() method in chromite/cbuildbot/commands.py to pick the updaters.sh under the firmware directory, i.e. $buildroot/chroot/build/$board/firmware, and reuse the same logic to get the versions via "updater.sh -V".

We really want to get this fixed so we have proper data, but what's intriguing is why this works for other branches but not for the glados one?

Take a look at GoldenEye here https://cros-goldeneye.corp.google.com/console/listFFBuild?type=firmware&branch=firmware-glados-7820.B#/details and click on any link on the "Build status" column, there you will find the kunimitsu group is OK but not the glados group, which has different FW versions for glados and chell.

Did something else change? I'd like to be sure we understand the problem before fixing it.

Thank you!

+bernie might have insight into the version mismatch for chell.

It is a good question why other branches are OK but chell. I investigated a bit.

It is because we released a firmware updater of chell before we forked the firmware branch. You can check the chell updater ebuild, i.e.

https://chrome-internal.googlesource.com/chromeos/overlays/overlay-chell-private/+/firmware-glados-7820.B/chromeos-base/chromeos-firmware-chell/chromeos-firmware-chell-9999.ebuild

CROS_FIRMWARE_MAIN_IMAGE="bcs://Chell.7723.0.0.tbz2"
CROS_FIRMWARE_EC_IMAGE="bcs://Chell_EC.7723.0.0.tbz2"
CROS_FIRMWARE_PD_IMAGE="bcs://Chell_PD.7723.0.0.tbz2"

And the kunimitsu updater ebuild, i.e.

https://chrome-internal.googlesource.com/chromeos/overlays/overlay-kunimitsu-private/+/firmware-glados-7820.B/chromeos-base/chromeos-firmware-kunimitsu/chromeos-firmware-kunimitsu-9999.ebuild

CROS_FIRMWARE_MAIN_IMAGE=""
CROS_FIRMWARE_EC_IMAGE=""


If no updater released, like the kunimitsu's empty strings, it will use the firmware built from source, that is the same version of new built firmware. It makes the metadata.json firmware version as same as the new built firmware version. It looks correct.

In most of the cases, we released a firmware updater after forked the branch. So the metadata.json firmware version looks correct. But in the chell case, we released a firmware updater before forked the branch. It causes the problem. So the metadata.json firmware version should not come from the updater; should look at the new built firmware.

Would your fix work correctly for all branches (release, firmware, etc)? If yes, I guess we should move forward and make the change. Will also this mean it has to be cherry picked potentially to all branches?

If we see a firmware branch broken like this, we can go ahead and revert the setting of a 'stable' version in the chromeos-firmware-$BOARD ebuild which should fix this logic, assuming there are at least some cases we want to get the firmware version from the chromeos-firmware-$BOARD stable package. 

In most firmware branches we will not have a stable version set before the firmware branch, only a very early one like Chell might have this version set, and it does not hurt to modify it on the firmware branch since the firmware branch is not actually using the 'stable' firmware in any sort of OS build.

For instance, I think https://chrome-internal-review.googlesource.com/251892 should fix chell.

Bernie's comments make a lot of sense, but should we go with the suggested fix by Tom as well so new branches get the corrected code? Then we simply fix branches already created using Bernie's method when we see fit?

I checked in GoldenEye and there other FW branches that have a FW version older than the Chrome OS version which could be wrong as well. I just tested Samus and it's having the same problem, and now I'm thinking most will have this problem as per Bernie's comment.

My gut is telling me to fix the code instead and cherry pick to all branches if possible. Is that like a super human effort? Can this be done in a reasonable time frame?

Fixing the GetFirmwareVersions() in the build code is easier (just several line change) and the correct way which reflects the versions of the new built firmware, not the stable updater.

AFAIK, the build code always uses the master branch, so only changing the master branch is fine, don't need to cherry-pick to all branches. The build team can confirm and may help to review/test your change.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/overlays/overlay-chell-private/+/e3af6cc7041bbd79d8fa3b3c6642f214fe37644b

commit e3af6cc7041bbd79d8fa3b3c6642f214fe37644b
Author: Bernie Thompson <bhthompson@chromium.org>
Date: Thu Mar 24 00:58:54 2016

Thanks for the effort, Tom. We've decided it's best to go the ebuild route, we're going to highlight the mismatch in GoldenEye so those affected users go and fix the ebuild as Bernie just did. This mainly because we think the code change won't work in release branches and cherry picking will still be needed as chromite is also branched.

Closing this bug, inviting people in GoldenEye to request a change like the one by Bernie when needed.