Issue 866253  has been merged into this issue.

I don't quite know what "marked 'under test' for nami and cyan" means, but now it makes sense why changes to the kernel/v4.14 repository are "relevant" for boards that do not use the chromeos-4.14 kernel:

(a) $ equery-grunt depends turbostat
 * These packages depend on turbostat:
virtual/target-chromium-os-dev-1-r35 (power_management ? dev-util/turbostat)

(b) so, any board that has USE=power_management will build turbostat,

(c) as in OP, turbostat is actually built directly from source in the kernel/v4.14 repository

(d) so, any change to the kernel/v4.14 repository could in theory effect turbostat.

(e) therefore every change to kernel/v4.14 must be verified on all boards that USE=power_management (even if they don't use the chromeos-4.14 kernel)

=> groeck & diander's have felt this pain for a while, too, given their extensive experience on kernel/v4.4, because from 2017-02-03 [0] to 2018-03-29 [1] turbostat had been built using kernel/v4.4 and would therefore exposed v4.4 CLs similarly to all boards.  (prior that it was built from v3.8)

[0] dc1b2958e78 turbostat: Update turbostat to build from kernel v4.4
[1] 55495d8ff33 turbostat: Update to version from kernel v4.14

I am not actually familiar with turbostat, but it appears to be for for Intel devices:
DESCRIPTION="Intel processor C-state and P-state reporting tool"

If we really must build it as a cros_workon from our chromium os specific kernel/v4.14, perhaps we could restrict it to only Intel devices.

However, the only good reason I can think of that we would need this as a cros_workon ebuild is if we are carrying Chromium OS specific patches for turbostat in our kernel/v4.14 repository.  If we are just building stock turbostat, why isn't this an upstream / portage-stable ebuild?

There are no Chrome OS / Chromium OS related changes in turbostat.

Shortcut: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+log/master/dev-util/turbostat/turbostat-9999.ebuild

> I am not actually familiar with turbostat, but it appears to be for for Intel devices

That seems correct.

> therefore every change to kernel/v4.14 must be verified on all boards that USE=power_management (even if they don't use the chromeos-4.14 kernel)

I can understand turbostat's dependency on kernel/v4.14 because that's where its source comes from. On the other hand I don't understand turbostat's dependency on chromeos-4.14. Can you clarify?


BTW I'm not aware of any actual dependency between turbostat and the kernel upstream. I believe turbostat is hosted in the kernel repo only for the mere "convenience" of most of its users, saving them one git clone command... and the inconvenience and confusion of all distributions as seen here.

> There are no Chrome OS / Chromium OS related changes in turbostat.

the turbostat source relies on some arch/x86/include/asm/ headers which we have changed in our tree (as in backports), so from the pov of the CQ, that isn't true.

> I can understand turbostat's dependency on kernel/v4.14 because that's where its source comes from. On the other hand I don't understand turbostat's dependency on chromeos-4.14. Can you clarify?

kernel/v4.14/ tracks the chromeos-4.14 branch.

> BTW I'm not aware of any actual dependency between turbostat and the kernel upstream. I believe turbostat is hosted in the kernel repo only for the mere "convenience" of most of its users, saving them one git clone command... and the inconvenience and confusion of all distributions as seen here.

it's including headers out of arch/x86/include/asm/ for cpu-specific headers.  if turbostat were to duplicate those headers, it could be independent, but it's not.

maybe adding this to the turbostat-9999 ebuild would help as it wouldn't be uprevved nearly as often:
CROS_WORKON_SUBTREE="arch/x86/include/asm tools/power/x86/turbostat"

If #5 is correct, than 1) turbostat is not usable for kernel versions other than chromeos-4.14 because, after all, it hard-depends on kernel headers in that branch, and 2) adding CROS_WORKON_SUBTREE does not really help because it would ignore those essential dependencies on the modified kernel headers.

This assumes that turbostat can not be built from upstream sources or from vanilla v4.14.y, but must indeed be built from a chromeos branch due to chromeos-specific header file changes.

> If #5 is correct, than 1) turbostat is not usable for kernel versions other than chromeos-4.14 because, after all, it hard-depends on kernel headers in that branch, and 2) adding CROS_WORKON_SUBTREE does not really help because it would ignore those essential dependencies on the modified kernel headers.

CROS_WORKON_SUBTREE should work fine if you look at how the code is built and exactly how the files are included.  it isn't using all of include/, just two specific standalone headers.

> This assumes that turbostat can not be built from upstream sources or from vanilla v4.14.y, but must indeed be built from a chromeos branch due to chromeos-specific header file changes.

there are trade-offs.  it could be built from newer versions, but then when newer SoCs are released, we have to figure out how to get those into the tree.  either patching the ebuild, or leverage the patches that already have to be written & merged into the kernel tree.

Hi, I maintain turbostat.

Marc is correct, I keep the source in the kernel tree mostly for convenience.  We like to keep the MSR definitions in one place for the kernel, and turbostat does not want to re-define those.

When I hand somebody a copy of turbostat source, I give them a snapshot
of turbostat.c and the two kernel headers that it uses.  They can take that
source and built it anywhere, and it will run on any kernel version --
newer or older.

I plan for future versions of turbostat to be similarly compatile
with newer and older kernels.

I hope this helps with your packaging issue.  Yes, I've had to tell
other distros also that it is not necessary for them to bind the version
of turbostat to a specific kernel tree.  The main thing is that you
generally want to be running the latest version you can get your hands on.

> I am not actually familiar with turbostat, but it appears to be for for Intel devices

Correction: Intel + AMD. Nothing else.

> it's including headers out of arch/x86/include/asm/ for cpu-specific headers.  

Just got confirmation that these specific kernel headers (the only ones turbostat depends on) are in the kernel tree because there was no better, central place to publish them. There used to be copies scattered in various places and constantly out of sync with each other and/or with slightly different register names.

> if turbostat were to duplicate those headers, it could be independent, but it's not.

Also got confirmation that it is indeed possible to extract just these headers from the kernel and build turbostat with just that. Some people do this from time to time; whenever it makes sense for them.

So who is going to fix this? Turbostat appears to have been added by an external contributor and then updated by Intel twice over the years. Is anything actually using this? If no one steps up to apply the suggested fix, it might be best to remove the package from the repo.

Turbostat is very much in use.

> This package is marked 'under test' for nami and cyan. Hence, the CL was marked relevant when it shouldn't really be (because those devices aren't running kernel 4.14).

Simple, detailed reproduction steps that do not require access to Google's internal infrastructure would help. Please also keep in mind Gentoo is arcane for most people, thanks!

We're still actively using turbostat, and need the latest version available (which means the version from the latest kernel sources).

vapier's suggestion to set CROS_WORKON_SUBTREE sounds promising, since any changes to the latest kernel sources outside of the turbostat code shouldn't be relevant to builds that only pull in turbostat.

my one-line suggestion in comment #5 can be posted+tested by anyone ...

You won't be able to reproduce the failure locally but, in this case, that doesn't matter. All that needs to be done is to follow the recommendation in #5 and #7 to update the ebuild. After that, if it builds locally and it passes PreCQ and CQ, that should fix it. We'll confirm once it lands.

> You won't be able to reproduce the failure locally but, in this case, that doesn't matter.

Not the failure (I didn't get there is a build "failure", is there?) but at least the build steps to observe the spurious build(s) triggered by the spurious dependenci(es). Isn't that what this bug is about? These (mandatory) TEST= steps would give whoever fixes this: 1. some idea of what he or she will be doing and why; 2. a means to verify that he or she actually addresses the issue.

> my one-line suggestion in comment #5 can be posted+tested by anyone ...

... by any of the rare people with a good enough understanding of what's going on (not including me). If this issue can't be dumbed down to a list of TEST= steps then it can't be fixed by any random person.

I'm working on externalizing our infrastructure so that Intel will be able to access it sometime in late 2019. However, even for those of us with access to the infrastructure, this bug is subtle that it took us months to root cause it and, even then, we cannot reproduce it locally: we have to submit the CL to see if it fixes the problem.

regardless of the relevance detection, CROS_WORKON_SUBTREE is a useful optimization as it'll cut down on the number of times the package is uprevved.  atm it uprevs for every kernel CL instead of just CLs that update turbostat code.

wrt testing, you can verify that like any other cros_workon 9999 package.  or upload it and let the trybots verify it.

I'll bite...  CL for #5:
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1150864

Well, that didn't work:
https://luci-logdog.appspot.com/logs/chromeos/buildbucket/cr-buildbucket.appspot.com/8939980981142177152/+/steps/BuildPackages/0/stdout#

turbostat-4.14.57-r2030: >>> Emerging (1 of 1) dev-util/turbostat-4.14.57-r2030::chromiumos for /build/eve/
turbostat-4.14.57-r2030:  * Running stacked hooks for pre_pkg_setup
turbostat-4.14.57-r2030:  *    sysroot_build_bin_dir ...
turbostat-4.14.57-r2030:  [ ok ]
turbostat-4.14.57-r2030:  * Running stacked hooks for post_pkg_setup
turbostat-4.14.57-r2030:  *    python_eclass_hack ...
turbostat-4.14.57-r2030:  [ ok ]
turbostat-4.14.57-r2030:  * Running stacked hooks for pre_src_unpack
turbostat-4.14.57-r2030:  *    python_multilib_setup ...
turbostat-4.14.57-r2030:  [ ok ]
turbostat-4.14.57-r2030: >>> Unpacking source...
turbostat-4.14.57-r2030:  * Using local source dir(s): /mnt/host/source/src/third_party/kernel/v4.14
turbostat-4.14.57-r2030:  * path: /mnt/host/source/src/third_party/kernel/v4.14
turbostat-4.14.57-r2030:  * destdir: /build/eve/tmp/portage/dev-util/turbostat-4.14.57-r2030/work/turbostat-4.14.57
turbostat-4.14.57-r2030: >>> Source unpacked in /build/eve/tmp/portage/dev-util/turbostat-4.14.57-r2030/work
turbostat-4.14.57-r2030:  * Running stacked hooks for post_src_unpack
turbostat-4.14.57-r2030:  *    asan_init ...
turbostat-4.14.57-r2030:  [ ok ]
turbostat-4.14.57-r2030: >>> Preparing source in /mnt/host/source/src/third_party/kernel/v4.14 ...
turbostat-4.14.57-r2030: >>> Source prepared.
turbostat-4.14.57-r2030: >>> Configuring source in /mnt/host/source/src/third_party/kernel/v4.14 ...
turbostat-4.14.57-r2030:  * ACCESS DENIED:  opendir:      /mnt/host/source/src/third_party/kernel/v4.14/kernel
turbostat-4.14.57-r2030: find: `/mnt/host/source/src/third_party/kernel/v4.14/kernel': Permission denied
turbostat-4.14.57-r2030:  * ACCESS DENIED:  opendir:      /mnt/host/source/src/third_party/kernel/v4.14/scripts
turbostat-4.14.57-r2030: find: `/mnt/host/source/src/third_party/kernel/v4.14/scripts': Permission denied
turbostat-4.14.57-r2030:  * ACCESS DENIED:  opendir:      /mnt/host/source/src/third_party/kernel/v4.14/samples
turbostat-4.14.57-r2030: find: `/mnt/host/source/src/third_party/kernel/v4.14/samples': Permission denied
turbostat-4.14.57-r2030:  * ACCESS DENIED:  opendir:      /mnt/host/source/src/third_party/kernel/v4.14/security
turbostat-4.14.57-r2030: find: `/mnt/host/source/src/third_party/kernel/v4.14/security': Permission denied
turbostat-4.14.57-r2030:  * ACCESS DENIED:  opendir:      /mnt/host/source/src/third_party/kernel/v4.14/crypto


I won't have time to debug this again for a while.  Hope someone else can step up here in the interim.

that failure is because our eclasses are trying to be too smart

you can stub it out in the turbostat ebuild by adding:

 using_common_mk() { return 1; }

we should prob rip out the common.mk logic from cros-workon as we no longer standardize on it anymore ... we have platform for that.  i've filed  issue 867926  to track that.

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

commit 6603c1de193ae95b8bf517caa1729f12ea13f24b
Author: Daniel Kurtz <djkurtz@chromium.org>
Date: Sat Aug 04 09:00:05 2018

dev-util/turbostat: Add CROS_WORKON_SUBTREE

Currently, turbostat is built directly from sources in the Chromium OS
kernel/v4.14 repository.  Since it was a cros_workon package without
a CROS_WORKON_SUBTREE, any change to kernel/v4.14 would also cause
the turbostat package to be rebuilt.

However, all of the turbostat source code is built from just two source
directories, hence we can use CROS_WORKON_SUBTREE to narrow the scope of
changes that would cause a rebuild.

Also, work around our eclasses are trying to be too smart by stubbing out
common.mk logic as suggested by vapier.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

BUG= chromium:863077 
TEST=Land this CL, CQ a kernel/v4.4 change that does not modify anything
  in arch/x86/include/asm or tools/power/x86/turbostat.
  => The kernel/v4.14 CLs should not be marked as DetectRelevantChanges
  for boards that do not use chromeos-kernel-4_14.

Change-Id: I1e0fb34fc664ac80bfa63f30d75acefd66ade2ad
Reviewed-on: https://chromium-review.googlesource.com/1150864
Commit-Ready: Daniel Kurtz <djkurtz@chromium.org>
Tested-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/6603c1de193ae95b8bf517caa1729f12ea13f24b/dev-util/turbostat/turbostat-9999.ebuild

Fixed?

Presumably fixed by #22, but I never did get around to verifying.
I'd appreciate some help if someone could check:
TEST=Land this CL, CQ a kernel/v4.14 change that does not modify anything
  in arch/x86/include/asm or tools/power/x86/turbostat.
  => The kernel/v4.14 CLs should not be marked as DetectRelevantChanges
  for boards that do not use chromeos-kernel-4_14.

Verification:

[0] was exonerated for HWTest failures on non-chromeos-4.14 kernels.

[0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1183641