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

Issue 740549 link

Starred by 1 user

Issue metadata

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

Blocked on:
issue 813791



Sign in to add a comment

trunks package failing to compile due to broken #include <gmock/gmock.h>

Project Member Reported by derat@chromium.org, Jul 10 2017

Issue description

The last two builds of reef-chrome-pfq at https://uberchromegw.corp.google.com/i/chromeos/builders/reef-chrome-pfq have failed in BuildPackages due in part to problems with the chromeos-base/trunks package:

trunks-0.0.1-r1909: FAILED: aosp/system/tpm/trunks/trunks_test.mock_authorization_delegate.o 
trunks-0.0.1-r1909: x86_64-cros-linux-gnu-clang++ -MMD -MF aosp/system/tpm/trunks/trunks_test.mock_authorization_delegate.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -I/build/reef/tmp/portage/chromeos-base/trunks-0.0.1-r1909/work/trunks-0.0.1/aosp/system/tpm -Iaosp/system/tpm/trunks/trunks_test.gen/include -Igen/include -I/build/reef/tmp/portage/chromeos-base/trunks-0.0.1-r1909/work/trunks-0.0.1/platform2 -I/build/reef/tmp/portage/chromeos-base/trunks-0.0.1-r1909/work/trunks-0.0.1/platform -I/build/reef/usr/include -Wall -Wno-psabi -Wunused -Wno-unused-parameter -ggdb3 -fstack-protector-strong -Wformat=2 -fvisibility=internal -Wa,--noexecstack -Werror --sysroot=/build/reef -DUSE_RTTI_FOR_TYPE_TAGS -Wno-c++11-extensions -Wno-unused-local-typedefs -DBASE_VER=395517 -pthread -I/build/reef/usr/include/chromeos -I/build/reef/usr/include/base-395517 -I/build/reef/usr/include/glib-2.0 -I/build/reef/usr/lib64/glib-2.0/include -I/build/reef/usr/include/nss -I/build/reef/usr/include/nspr -I/build/reef/usr/include/dbus-1.0 -I/build/reef/usr/lib64/dbus-1.0/include -fPIE -std=gnu++11 -DVCSID='"0.0.1-r1909-52b19713e49d1867a2c44ea11d7089e900d8730e"' -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -O2 -pipe -O2 -pipe -O2 -pipe -march=corei7 -g -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -clang-syntax -clang-syntax -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables  -c ../../../../../../../tmp/portage/chromeos-base/trunks-0.0.1-r1909/work/trunks-0.0.1/aosp/system/tpm/trunks/mock_authorization_delegate.cc -o aosp/system/tpm/trunks/trunks_test.mock_authorization_delegate.o
trunks-0.0.1-r1909: In file included from ../../../../../../../tmp/portage/chromeos-base/trunks-0.0.1-r1909/work/trunks-0.0.1/aosp/system/tpm/trunks/mock_authorization_delegate.cc:17:
trunks-0.0.1-r1909: /build/reef/tmp/portage/chromeos-base/trunks-0.0.1-r1909/work/trunks-0.0.1/aosp/system/tpm/trunks/mock_authorization_delegate.h:23:10: fatal error: 'gmock/gmock.h' file not found
trunks-0.0.1-r1909: #include <gmock/gmock.h>
trunks-0.0.1-r1909:          ^~~~~~~~~~~~~~~
trunks-0.0.1-r1909: 1 error generated.

I think that the problem is that trunks.gyp defines a trunks_test target that isn't hidden behind a check for the test USE flag.
 

Comment 1 by derat@chromium.org, Jul 10 2017

Speculative fix at http://crrev.com/c/565043.

I'm just adding the missing dependency, but I'm not sure that libtrunks_test.a is actually used by anything. (I also don't know why test code is getting installed in production images in the first place.)
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 10 2017

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

commit ba2feb1f0a849d21fae99e4fe222025d2c103859
Author: Daniel Erat <derat@chromium.org>
Date: Mon Jul 10 18:26:22 2017

trunks: Add gmock to DEPEND.

The trunk package unconditionally installs libtrunks_test.a,
which includes code that depends on gmock.

BUG= chromium:740549 
TEST=none

Change-Id: I807dbab91e38f9332585df38eefd86625a3a048e
Reviewed-on: https://chromium-review.googlesource.com/565043
Commit-Queue: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Greg Levin <glevin@chromium.org>

[modify] https://crrev.com/ba2feb1f0a849d21fae99e4fe222025d2c103859/chromeos-base/trunks/trunks-9999.ebuild

libtrunks_test.a is used by aosp/system/tpm/{attestation,tpm_manager}. They link to it for common things like TrunksFactoryForTest when building attestation_testrunner/tpm_manager_testrunner.

Since attestation/tpm_manager can be tested independently from trunks, they can have USE=test when trunks was built w/o USE=test.

Is there a better way to achieve having a common test lib built by one package and used by another?
Cc: apronin@chromium.org

Comment 5 by derat@chromium.org, Jul 20 2017

My understanding is that setting FEATURES=test when running emerge both sets the test USE flag and results in the src_test phase being run.

Can we make the attestation and tpm_manager ebuilds require that trunks was built with USE=test when they themselves are built with USE=test?

Alternately, it's probably cleaner to introduce a new trunks-test package that builds and installs libtrunks_test.a (or better, make it into a shared library). Then attestation and tpm_manager can just depend on that when they're built with USE=test.
> Can we make the attestation and tpm_manager ebuilds require that trunks was built with USE=test when they themselves are built with USE=test?

Should that work with UnitTest stage run by builders? Do they just emerge everything with FEATURES=test?

> Alternately, it's probably cleaner to introduce a new trunks-test package that builds and installs libtrunks_test.a (or better, make it into a shared library). Then attestation and tpm_manager can just depend on that when they're built with USE=test.

Trunks own unit tests also need libtrunks_test.a. We could also move them (trunks_testrunner target in trunks.gyp) to that trunks-test package, but that itself may be confusing to those who want to run those tests manually against the trunks package after making a change.
Btw, I do see mocks (though, just .h) installed as a part of non-test builds in other cases. Any ebuild that uses platform_install_dbus_client_lib (e.g power_manager-client): does 
doins "${OUT}/gen/include/${libname}/dbus-proxy-mocks.h"

Maybe it's just a question of choosing a better name? Something like libtrunks_test -> libtrunks_client_mocks.

libtrunks_test consists of mocks for various interface classes + an implementation of TrunksFactory that returns those mocks (or lets the tests inject their own instances).

Comment 8 by derat@chromium.org, Jul 21 2017

No, I don't think it's a naming problem. We shouldn't be including code that's only used for testing (including mock implementations) in non-test images. It's a waste of space, especially when it means we also pull in other test-specific libraries like gmock.

Headers aren't a problem as far as I'm aware since we strip them out from the image.

Comment 9 by vapier@chromium.org, Jul 31 2017

in the attestation/tpm_manager ebuilds, write the dependency like:
  chromeos-base/trunks[test?]

that should force portage to DTRT here.  then you can make all the code in trunks conditional upon USE=test.
A related issue 760657 - at least the "tpm_manager missing gmock/gmock.h" part. Btw, trunks is not built for tpm 1.2 ("+tpm -tpm2").

I do see gmock in BuildPackages, though:
[binary  N     ] dev-cpp/gmock-1.8.0 to /build/whirlwind/ USE="{-test}" 473 KiB
Fetched dev-cpp/gmock-1.8.0 in 6.36s

So, maybe it's the "USE=-test" part, and tpm_manager shouldn't build mocks in this case? That may trigger further missing dependencies on tpm_manager mocks when "-test" (from attestation, cryptohome, ap-daemons, anything else?), so will need to trace all.

Comment 11 by derat@chromium.org, Aug 30 2017

I tried to fix the dependency in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/565960, but it produces build errors that I don't fully understand.
Project Member

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

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

commit f30e1ea61ffd70600ff4f66ea7885510bbb3ce2a
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Oct 06 22:51:10 2017

cros_run_unit_tests: drop --nodeps --buildpkgonly

This breaks our ability to pull in packages based on USE=test and
otherwise change their build time behavior.

These options have been enabled since the beginning of this script.
Before CL:1684, we used `ebuild` to manually run the test phase on
specific packages and avoid building/merging binpkgs.  After that,
we switched to emerge and --nodeps/--buildpkgonly to largely get
the same behavior.

By dropping --nodeps, we allow additional packages to get pulled in
that otherwise would require manual listing in the deptree, either
as an unconditional dep in the package, or in a common virtual which
doesn't scale.  It also means we run the test phases for those deps,
but we don't have any packages (yet?) that this matters to.  We can
improve this later on by manually building the depgraph of test deps
as part of the main build_packages phase.

By dropping --buildpkgonly, we allow packages to adjust things they
install based on the USE=test flag.  This adds a some overhead where
we are merging the packages into the sysroot (again).  This may lead
to a slight desync in that the sysroot has files that aren't in the
release image, but we're already in that situation now with all the
install masked files we strip out.

BUG= chromium:740549 
TEST=precq run rebuilds packages as needed and pulls in new deps

Change-Id: I7a8b1eb1792fe6b6d0b80b53020fac2eff952311
Reviewed-on: https://chromium-review.googlesource.com/698948
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/f30e1ea61ffd70600ff4f66ea7885510bbb3ce2a/lib/chroot_util.py

Project Member

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

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

commit 258c5851889feb057bb79f048799b439c25deba0
Author: Daniel Erat <derat@chromium.org>
Date: Sat Oct 07 22:33:38 2017

trunks: Only install libtrunks_test.a for test USE flag.

This test library was getting installed unconditionally,
seemingly as part of now-outdated Brillo stuff
(http://b/24099462). It only appears to be used by other
packages when the "test" USE flag is set.

Stop installing it by default so a followup change to
trunks.gyp can make us stop building it by default, so
another change to this ebuild can make the trunks package
stop unconditionally depending on gmock.

Also make the attestation, chaps, cryptohome, and
tpm_manager packages, all of which depend on
libtrunks_test.a when built for testing, require that trunks
be built with the "test" USE flag when they themselves are
built with it.

BUG= chromium:740549 
TEST=built a dev image for a board with tpm2

Change-Id: I824547721d6fc60d215473fac9f50856165dcc64
Reviewed-on: https://chromium-review.googlesource.com/565960
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/258c5851889feb057bb79f048799b439c25deba0/chromeos-base/tpm_manager/tpm_manager-9999.ebuild
[modify] https://crrev.com/258c5851889feb057bb79f048799b439c25deba0/chromeos-base/chaps/chaps-9999.ebuild
[modify] https://crrev.com/258c5851889feb057bb79f048799b439c25deba0/chromeos-base/trunks/trunks-9999.ebuild
[modify] https://crrev.com/258c5851889feb057bb79f048799b439c25deba0/chromeos-base/attestation/attestation-9999.ebuild
[modify] https://crrev.com/258c5851889feb057bb79f048799b439c25deba0/chromeos-base/cryptohome/cryptohome-9999.ebuild

Project Member

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

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/tpm/+/8abfbae298ce3689a6c6be68d7094e2841968aec

commit 8abfbae298ce3689a6c6be68d7094e2841968aec
Author: Daniel Erat <derat@chromium.org>
Date: Mon Oct 09 01:30:13 2017

trunks: Only build libtrunks_test.a for test USE flag.

BUG= chromium:740549 
TEST=none

Change-Id: I3ea20d8532667429fe8038b1746401365276871a
Reviewed-on: https://chromium-review.googlesource.com/706702
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/8abfbae298ce3689a6c6be68d7094e2841968aec/trunks/trunks.gyp

Project Member

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

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

commit 503da065b9303a99c7d4748e84dd0c95e4adfa8f
Author: Daniel Erat <derat@chromium.org>
Date: Mon Oct 09 16:19:34 2017

trunks: Remove unconditional gmock dependency.

chromeos-base/trunks used to depend unconditionally on
dev-cpp/gmock. Now that libtrunks_test.a is only built when
the "test" USE flag is set, remove the unconditional gmock
dependency. (The dependency is still added by
platform.eclass when the test USE flag is set.)

BUG= chromium:740549 
TEST=none

Change-Id: I32c89186928de472c6c1974ac961a2dc7104b86c
Reviewed-on: https://chromium-review.googlesource.com/706308
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/503da065b9303a99c7d4748e84dd0c95e4adfa8f/chromeos-base/trunks/trunks-9999.ebuild

Status: Verified (was: Started)
Looks like after https://crrev.com/c/706702 we now can't unittest cryptohome for tpm2 (without first building a test version of trunks):
# cros_workon_make --board=eve --test cryptohome
...
/usr/x86_64-pc-linux-gnu/x86_64-cros-linux-gnu/binutils-bin/2.27.0/ld.gold.real: error: cannot find -ltrunks_test
...

"cros_run_unit_tests --board eve --packages cryptohome" succeeds. After all, it emerges dependencies with USE="{test*}". It doesn't run the unittests themselves, though. At least, not for cryptohome. Is that expected?

And running "cros_workon_make --board=eve --test cryptohome" after "cros_run_unit_tests --board eve --packages cryptohome" also works (no surprise here as well).

So, what's the right way to run unittests for cryptohome with these changes?

And does running unit tests now interferes with build_packages - should I rebuild packages after doing unittests to make sure that the resulting image contains non-test-versions of those packages?
cros_workon_make ignores the dep tree. it's got sharp edges like this.

I don't see any conflict with build_packages.
Blockedon: 813791

Sign in to add a comment