New issue
Advanced search Search tips

Issue 765753 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 405595
issue 679858
issue 764753



Sign in to add a comment

chromeos-postinst: run inside of the target root instead of outside

Project Member Reported by vapier@chromium.org, Sep 15 2017

Issue description

currently /postinst looks like:
  INSTALL_ROOT=$(dirname "$0")
  ${INSTALL_ROOT}/usr/bin/cros_installer postinst "${INSTALL_ROOT}" $@

and then all the things that "postinst" runs are expected to manually prefix $INSTALL_ROOT and load things out of that.

what if we changed /postinst to do:
  INSTALL_ROOT=$(dirname "$0")
  chroot "${INSTALL_ROOT}" /usr/bin/cros_installer postinst "/" $@

then we wouldn't have to worry about static linkage at all
 
Blocking: 679858
I see no issues with that as long as the firmware updaters are okay, but the current owners should speak up.

Comment 3 by vapier@chromium.org, Sep 15 2017

Blocking: 405595
lots of good old discussion in  issue 405595 .  i don't think anything in there says we can't do this.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 18 2017

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

commit cca0aad2b38c8e06172612cb6a699a0d926b3bb1
Author: Mike Frysinger <vapier@chromium.org>
Date: Mon Sep 18 21:55:01 2017

installer: update OWNERS

BUG= chromium:765753 
TEST=read it

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

[modify] https://crrev.com/cca0aad2b38c8e06172612cb6a699a0d926b3bb1/installer/OWNERS

Comment 5 by hungte@chromium.org, Sep 19 2017

I remember we did chroot in the beginning, and then had an issue that new rootfs does not promise to work for old kernel, for reasons like 32-64 migration, and then changed to current way so we can try both new and old rootfs.

Not sure if it's safe today to go back to chroot world. You do need to watch AU results for this.
The new rootfs has to be mountable by the old kernel, or we wouldn't be able to get to postinst at all.

That doesn't mean there wasn't some other goofy restriction though.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/73591187e1f7afb7c254d1c21d88b3eac96acfc2

commit 73591187e1f7afb7c254d1c21d88b3eac96acfc2
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Oct 06 04:24:43 2017

installer: change postinst to run inside the installed path

All the postinst logic manually adds the specified path to the prefix of
the tools it runs in order to find the right binary.  But we run into the
problem of running dynamic programs against a different runtime.  To work
around this, we statically linked things, but that's problematic:
- we sometimes forget to statically link programs
- it's hard to know what all programs actually need to be
- shell scripts make this problem much worse
- mixing of static & dynamic causes disk bloat
- static linkage bleeds into other targets too (firmware updater/initramfs)

Lets change the postinst code so that it runs in the target root as any
other chroot.  All of our dynamic linkage issues go away as a result.

BUG= chromium:765753 
TEST=precq passes
TEST=recovery image passes
TEST=cros flash passes

Change-Id: I9d3fa9b6f419ada75e8f94b9dd0260fcef486b64
Reviewed-on: https://chromium-review.googlesource.com/669013
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/73591187e1f7afb7c254d1c21d88b3eac96acfc2/installer/chromeos-postinst

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 12 2017

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

commit e47d57751b3c3bbf0c201231efea48eb307ffb92
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Oct 12 01:32:49 2017

installer: also bind mount /var and /mnt

Some of the postinst steps touch paths under /var and
/mnt/stateful_partition, so bind mount those too.

BUG= chromium:765753 
TEST=precq passes
TEST=recovery image passes

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

[modify] https://crrev.com/e47d57751b3c3bbf0c201231efea48eb307ffb92/installer/chromeos-postinst

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 23 2017

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

commit 75cae89af502f99dc7de8d7a82f84542e789e0dd
Author: Mike Frysinger <vapier@chromium.org>
Date: Mon Oct 23 22:22:19 2017

chromeos-installer: convert to gyp

BUG= chromium:765753 
TEST=precq passes
CQ-DEPEND=CL:669015

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

[modify] https://crrev.com/75cae89af502f99dc7de8d7a82f84542e789e0dd/chromeos-base/chromeos-installer/chromeos-installer-9999.ebuild

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 24 2017

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

commit 4dd796c3418b7861ce81bf9ea45c734be65332a9
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Oct 24 22:30:14 2017

gmock/gtest: drop static linkage

Now that the installer is dynamically linked, we no longer need these
static libs for testing.

BUG= chromium:765753 
TEST=precq passes
CQ-DEPEND=CL:669359

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

[modify] https://crrev.com/4dd796c3418b7861ce81bf9ea45c734be65332a9/profiles/targets/chromeos/package.use

Labels: OS-Chrome
Owner: vapier@chromium.org
Status: Fixed (was: Available)
this seems to be sticking now, so going to call it fixed
SGTM. I don't see any problem about switching between 32 and 64 userspace in this setup as long as the old kernel knows how to runs either binary.
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 6 2017

Labels: merge-merged-factory-eve-9667.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/10d9624fb2e3cc45c4204c1ec60fb7d35843a328

commit 10d9624fb2e3cc45c4204c1ec60fb7d35843a328
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Dec 06 19:33:13 2017

chromeos-installer: convert to gyp

BUG= chromium:765753 
TEST=precq passes
CQ-DEPEND=CL:669015

Change-Id: I650298454a669c52e90c46f66aa3b3caf7759925
Reviewed-on: https://chromium-review.googlesource.com/669359
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
(cherry picked from commit 75cae89af502f99dc7de8d7a82f84542e789e0dd)
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/809525

[modify] https://crrev.com/10d9624fb2e3cc45c4204c1ec60fb7d35843a328/chromeos-base/chromeos-installer/chromeos-installer-9999.ebuild

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 6 2017

Comment 16 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 17 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/firmware/+/0e3c59515fcce2931d5503bf6d89237b10f2ca52

commit 0e3c59515fcce2931d5503bf6d89237b10f2ca52
Author: Hung-Te Lin <hungte@chromium.org>
Date: Sun Sep 09 17:33:52 2018

pack_firmware: Change default tools to empty for updater5

Since chromium:765753, the postinst now always runs updater inside
the new rootfs (using chroot) so bundling tool programs (flashrom,
crossystem, mosys, ...) is no longer needed for AU and recovery.

There may be still developers grabbing an older/newer version
of chromeos-firmwareupdate and run it directly on different rootfs,
where the related programs may be not compatible and cause updating to
fail -- especially in early proto stages. However, this may be not super
critical to support, especially since, we have zero testing/validation
making sure we actually bundle all the binaries we care about (for
example, mosys calls other programs like vpd_get_value). Plus, currently
we are moving updating logic into futility itself (chromium:875551).

As a result, in this patch we are going to experiment "no tools bundled"
on new devices using updater5 (which will run the futility based
updater) and then decide if we should remove all tools for all devices.

BUG= chromium:765499 , chromium:765753 , chromium:875551 
TEST=./pack_firmware_unittest.py
     ./pack_firmware_functest.py
     Set CROS_FIRMWARE_SCRIPT=updater5.sh in ebuild and then
     emerge-$BOARD chromeos-firmware-$BOARD;
     /build/$BOARD/usr/sbin/chromeos-firmwareupdate -V # no tools

Change-Id: I5b388129ac3961ef5a52767b861cb70bbee596c9
Reviewed-on: https://chromium-review.googlesource.com/1215102
Commit-Ready: Hung-Te Lin <hungte@chromium.org>
Tested-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/0e3c59515fcce2931d5503bf6d89237b10f2ca52/pack_firmware_unittest.py
[modify] https://crrev.com/0e3c59515fcce2931d5503bf6d89237b10f2ca52/pack_firmware.py

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/firmware/+/2cd6f5a4299d77480847f5f27676e20aabd719d7

commit 2cd6f5a4299d77480847f5f27676e20aabd719d7
Author: Hung-Te Lin <hungte@chromium.org>
Date: Thu Nov 01 17:02:15 2018

pack_firmware: Remove --tool and deprecate --tool_base

The design of bundling programs (especially those exist inside rootfs,
i.e., flashrom, crossystem, mosys, ...) in firmware updater was
introduced long ago when the underlying programs were still changing
rapidly, and updater was full of shell scripts.

Recently we are seeing challenges that
 - The duplicated programs have increased updater size rapidly
 - There is no easy way to ensure all needed programs (and all
   dependencies, including library, data and other executable files)
   are included.

Since then, there were few issues created to solve the problem,
including:
 - chromium:765753 changed the postinst to always run inside chroot
 - chromium:765499 tries to copy dynamically linked programs with lddtree

This change has removed --tool and deprecated --tool_base (which is
currently still used by cros-firmware.eclass, and can be removed after
this is merged), so the updater script will always find programs from
rootfs. If really needed, a device may still include additional programs
using --extra.

- Auto update and recovery will run the binaries from new rootfs.
- Factory installer may need to upgrade related tools (flashrom, mosys,
  etc) but that's probably not a big problem since there are other
  programs using mosys, so the factory branch needs to cherry-pick
  related (especailly unibuild) changes as well.
- Developers working on early builds may suffer if there are known
  issues in flashrom or crossystem, but they can always flash using
  servo, or boot a fresh new disk image and run new tools from there.

BUG= chromium:765499 , chromium:765753 
TEST=./pack_firmware_unittest.py
     ./pack_firmware_functest.py

Change-Id: Icbc98953f6a13d3f1a4f248f3643b6c77e8e4956
Reviewed-on: https://chromium-review.googlesource.com/1212684
Commit-Ready: Hung-Te Lin <hungte@chromium.org>
Tested-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[delete] https://crrev.com/85a7b3eb388b1fa826179617eaae6593dd290cf6/test/flashrom
[delete] https://crrev.com/85a7b3eb388b1fa826179617eaae6593dd290cf6/test/futility
[modify] https://crrev.com/2cd6f5a4299d77480847f5f27676e20aabd719d7/functest/bin/flashrom
[modify] https://crrev.com/2cd6f5a4299d77480847f5f27676e20aabd719d7/pack_firmware_unittest.py
[modify] https://crrev.com/2cd6f5a4299d77480847f5f27676e20aabd719d7/pack_firmware.py
[modify] https://crrev.com/2cd6f5a4299d77480847f5f27676e20aabd719d7/pack_firmware_functest.py
[delete] https://crrev.com/85a7b3eb388b1fa826179617eaae6593dd290cf6/local_flashrom/flashrom

Sign in to add a comment