New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 737210 link

Starred by 0 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

duplicate code in update_engine/scrips/update_payload and platform/dev/host/lib/update_payload

Project Member Reported by ahass...@chromium.org, Jun 27 2017

Issue description

The recent update engine merge from AOSP, also merged in the udpate_payloads code, which have been copied to update_engine from platform/dev/host/lib/update_payload used by Android folks to be used in Android side. But now we have two version of the (almost) same code one in update_engine/scrips/update_payload and one in platform/dev/host/lib/update_payload. I would like to change the servers to use the code in update_engine for two reasons: First, it is not good to have duplicate codes, and second if we do any changes in the update_engine, we might forget to apply relevant changes in platform/dev/host/lib/update_payload and that might break canary since this code is not run in CQ I think. 

Does anyone know who owns platform/dev/host/lib/update_payload?

Thanks.
 
For dev payloads:

Not sure, grep around for cros_generate_update_payload. There are probably multiple locations.

For release payloads:

This script zips up all build artifacts needed to generate payloads. All you need to do is update which artifacts are included.

https://cs.corp.google.com/chromeos_public/src/scripts/build_library/generate_au_zip.py

If there are any changes to the command lines after artifacts are extracted from the zip, then you need to update the code that uses it here:

http://cs/chromeos_public/chromite/lib/paygen/paygen_payload_lib.py

Storing off these generation artifacts is somewhat convoluted, but allows us to generate additional payloads after a release builder completes with identical tools.

Labels: -Restrict-View-Google Arch-All
Owner: ahass...@chromium.org
Status: Started (was: Untriaged)
Hi,

I added two CLs to move this directory, but I'm not sure if I did it correctly:
https://chromium-review.googlesource.com/c/598435
https://chromium-review.googlesource.com/c/598434

derat@ how can I thoroughly test and verify these. Is EndToEndUpdate is enough or I should do different things for dev and release builds?
Cc: de...@chromium.org
Hi all,

So what I did till now was two aforementioned CL's in #2 which basically does:
- removed platform/dev/host/lib/update_payload
- updated platform/dev/autoupdate.py to use the new address
- updated platform/dev/host/paycheck.py to use the new address. However I don't see any script uses paycheck.py, we have a duplicate of paycheck.py in update_engine too. I'm guessing people use paycheck.py manually?? If that's the case, should I just remove the one in update_engine? AOSP is not using it either.
- updated all its references in chromite
- updated chromite/lib/auto_updater.py to copy the sources from the new repo into its tmp dir.

The current problem I have is the devserver ebuild which apparently copies the unittests of update_payload to somewhere but we don't have the update_payload in platform/dev anymore. Does anyone know how to do the migration here? 

I also could not find any reference to update_payload in generate_au_zip.py or au-generator.zip. But might get into problem later after I was able to actually build an image :P

Thanks for the help.
Hi all, I think I finally did it :) The trick was to add a new ebuild for update_payload scripts and their unittests. Then add them to devserver dependecies. I uploaded the patches and ready for review :)

https://chromium-review.googlesource.com/c/598434
https://chromium-review.googlesource.com/c/598435
https://chromium-review.googlesource.com/c/614389

Thanks

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 23 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/fa107d34b4e2d564e5c61a4e18b4798a0b3b6c7c

commit fa107d34b4e2d564e5c61a4e18b4798a0b3b6c7c
Author: Xixuan Wu <xixuan@chromium.org>
Date: Sat Sep 23 04:21:33 2017

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/manifest/+/8804f4b08e28fc9fc42f0afda8e63e61d32c0da0

commit 8804f4b08e28fc9fc42f0afda8e63e61d32c0da0
Author: Xixuan Wu <xixuan@chromium.org>
Date: Sat Sep 23 04:21:38 2017

Add devserver to groups of /asop/system/update_engine.

BUG= chromium:737210 
TEST=None

Change-Id: Ia2e7763c24428eb6e543aeacca99affa9947f8e5
Reviewed-on: https://chromium-review.googlesource.com/679814
Reviewed-by: Allen Li <ayatane@chromium.org>
Commit-Queue: Xixuan Wu <xixuan@chromium.org>
Tested-by: Xixuan Wu <xixuan@chromium.org>

[modify] https://crrev.com/8804f4b08e28fc9fc42f0afda8e63e61d32c0da0/full.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/ed083d006a6c5b0edb552f58848d7e075369ada5

commit ed083d006a6c5b0edb552f58848d7e075369ada5
Author: Amin Hassani <ahassani@google.com>
Date: Mon Sep 25 22:52:44 2017

paygen: Use the update_payload in update_engine

Currently there are two version of the (almost) same code in
  platform/dev/host/lib/update_payload
and
  aosp/system/update_engine/scripts

We would like to use aops/system/update_engine/scripts instead for two reasons:
  1) Not keeping duplicate code.
  2) Prevent mistakes like forgetting to update dev/host/lib when new changes
     are landed in the update engine itself. This can cause the CQ to be fine
     but canary builds will fail.

This patch changes all the update_payload references in chromite to
src/aosp/system/update_engine/scripts/update_payload.

BUG= chromium:737210 
TEST=tryjob --hwtest; cros flash; provision

Change-Id: I56d7155d0f3068112a46a55598b1881c87fc40e8
Reviewed-on: https://chromium-review.googlesource.com/598435
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Xixuan Wu <xixuan@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/ed083d006a6c5b0edb552f58848d7e075369ada5/cli/cros/cros_payload_unittest.py
[modify] https://crrev.com/ed083d006a6c5b0edb552f58848d7e075369ada5/lib/constants.py
[modify] https://crrev.com/ed083d006a6c5b0edb552f58848d7e075369ada5/cli/cros/cros_payload.py
[modify] https://crrev.com/ed083d006a6c5b0edb552f58848d7e075369ada5/lib/auto_updater.py
[modify] https://crrev.com/ed083d006a6c5b0edb552f58848d7e075369ada5/lib/paygen/paygen_payload_lib.py

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745

commit 3c6add6596c8c8d4ca6c3ae1a5936edb539ea745
Author: Amin Hassani <ahassani@google.com>
Date: Wed Sep 27 19:57:56 2017

devserver: migrate update_payload scripts to a new ebuild

Currently update_payload scripts are installed in devserver ebuild. But
since we are moving the use of update_payload scripts to the
update_engine itself, we need a new ebuild (update_payload) to install
these scripts and set it as one of the devserver dependencies.  This
patch increases the devserver version to 3.0.0.

Removes paycheck.py from cros-devutils since it is not used anywhere and
we have a proper copy of it in update_engine repo.

BUG= chromium:737210 
TEST=submitted a tryjob; cros flash <ip> <image>
CQ-DEPEND=CL:598435,CL:598434

Change-Id: Ie8c9bb1d2239fc12c1c054b87034a3dfee7596ae
Reviewed-on: https://chromium-review.googlesource.com/614389
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745/chromeos-base/devserver-deps/devserver-deps-0.0.1.ebuild
[add] https://crrev.com/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745/chromeos-base/update_payload/update_payload-9999.ebuild
[modify] https://crrev.com/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745/chromeos-base/cros-devutils/cros-devutils-9999.ebuild
[modify] https://crrev.com/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745/chromeos-base/devserver/devserver-9999.ebuild
[rename] https://crrev.com/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745/chromeos-base/devserver-deps/devserver-deps-0.0.1-r6.ebuild
[modify] https://crrev.com/3c6add6596c8c8d4ca6c3ae1a5936edb539ea745/chromeos-base/devserver/files/chromeos-version.sh

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/dev-util/+/51780c65cd22035f6d6f54e3c94bca47114756b6

commit 51780c65cd22035f6d6f54e3c94bca47114756b6
Author: Amin Hassani <ahassani@google.com>
Date: Wed Sep 27 19:57:56 2017

Use the update_payload in update_engine

Currently there are two version of the (almost) same code in
  platform/dev/host/lib/update_payload
and
  aosp/system/update_engine/scripts

This patch deletes all the update_payload scripts from the platform/dev and changes all the references to src/aosp/system/update_engine/scripts/update_payload.

BUG= chromium:737210 
TEST=cbuildbot --remote
CQ-DEPEND=CL:598435,CL:598435

Change-Id: I3cee23aa3d5ace826d2d6ba669f3bbc21f0a5ac1
Reviewed-on: https://chromium-review.googlesource.com/598434
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Xixuan Wu <xixuan@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/test_utils.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/format_utils.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/applier.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/format_utils_unittest.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/histogram.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/checker.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/update_metadata_pb2.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/paycheck.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/block_tracer.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/test_paycheck.sh
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/checker_unittest.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/payload.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/payload-test-key.pub
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/payload-test-key.pem
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/error.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/histogram_unittest.py
[modify] https://crrev.com/51780c65cd22035f6d6f54e3c94bca47114756b6/autoupdate.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/update-payload-key.pub.pem
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/__init__.py
[delete] https://crrev.com/84acd9d0695f64af03d057ee9b44ec44d1cd6884/host/lib/update_payload/common.py

Status: Verified (was: Started)
Marking this as verified.

Sign in to add a comment