Issue 623156 link

Starred by 2 users

Issue metadata

Status:	Archived
Owner:	----
Closed:	Sep 2016
Cc:	█ chongjiang@chromium.org sque@chromium.org
EstimatedDays:	----
NextAction:	----
OS:	Chrome
Pri:	3
Type:	Bug
cwp VerifyIn-55 VerifyIn-56 VerifyIn-57 VerifyIn-58 VerifyIn-59 VerifyIn-60 VerifyIn-61

quipper's external build needs Thread and Notification classes

Project Member Reported by dhsharp@chromium.org, Jun 24 2016

Issue description

Makefile.external won't work until compat Thread and Notification classes are provided for it.

Some possibilities:

- Force Makefile.external users to depend on libchrome, and use compat/cros/detail. Con: forcing a dependency on external users.

- Provide a compat/c++11/detail that uses std::thread to implement Thread, and std::condition_variable & mutex to implement a Notification. Con: std::thread/etc are banned by the style guide. otoh, this code would not be used by quipper when built for CrOS.

- The Thread and Notification classes only ended up being used in unit tests. Split those tests out (these tests also require running as root to be useful), and don't build them in Makefile.external.

I currently lean towards using std::thread.

Project Member

Comment 1 by bugdroid1@chromium.org, Jun 28 2016

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

commit af0998f00264dabe608f6462b61062b3ae3875da
Author: David Sharp <dhsharp@chromium.org>
Date: Fri Jun 24 19:13:49 2016

quipper: disable testing build with Makefile.external

The external build is going to be broken for a while until a better
solution for external users is found.

TEST=builds
BUG= chromium:623156 

Change-Id: I8e689caabb5d490adcf5e0797b0492f8e9527eef
Reviewed-on: https://chromium-review.googlesource.com/356181
Commit-Ready: David Sharp <dhsharp@chromium.org>
Tested-by: David Sharp <dhsharp@chromium.org>
Reviewed-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/af0998f00264dabe608f6462b61062b3ae3875da/chromeos-base/quipper/quipper-9999.ebuild

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 15 2016

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

commit 57d96c7731f9cb7473edbc0979cb4ebb22e9e802
Author: Wade Khadder <wgk@google.com>
Date: Thu Aug 04 21:38:36 2016

quipper: Changes to build with external makefile

Previously, the external makefile was left behind and no longer updated
during the work to add ARC++ support to quipper. Now we bring it up to date
so it can be built externally again.

- Get rid of dependency on StringPrintf
- Add compat/ext for external build
- Implement Thread and Notification using C++ standard library classes
- Update log_level path in Makefile
- Link in libelf
- Explicitly include unistd.h in dso.cc
- Include new sources in makefile

BUG= chromium:623156 
TEST=build successfully with Makefile.external

Change-Id: I815d18ba3a046cc758265985246a916c9bf7ae8b
Reviewed-on: https://chromium-review.googlesource.com/366470
Commit-Ready: Simon Que <sque@chromium.org>
Tested-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Chong Jiang <chongjiang@chromium.org>

[add] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/compat/ext/detail/thread.h
[modify] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/perf_parser.cc
[add] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/compat/ext/detail/proto.h
[modify] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/Makefile.external
[modify] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/dso_test_utils.cc
[add] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/compat/ext/detail/string.h
[modify] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/dso.cc
[add] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/compat/ext/detail/log_level.cc
[add] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/compat/ext/detail/test.h
[modify] https://crrev.com/57d96c7731f9cb7473edbc0979cb4ebb22e9e802/chromiumos-wide-profiling/compat/cros/detail/string.h