jaypipes / ghw

Go HardWare discovery/inspection library
Apache License 2.0
1.62k stars 174 forks source link

Add aarch64 support #333

Closed glimchb closed 1 year ago

glimchb commented 1 year ago

Authors

Co-authored by: Xingyou Chen rockrush@rockwork.org

Details

Tests

tested on 8 core machine:

$ uname -m
aarch64

$ go run cmd/ghwc/main.go cpu
cpu (1 physical package, 8 cores, 8 hardware threads)
 physical package #36 (8 cores, 8 hardware threads)
  processor core #0 (1 threads), logical processors [0]
  processor core #1 (1 threads), logical processors [1]
  processor core #2 (1 threads), logical processors [2]
  processor core #3 (1 threads), logical processors [3]
  processor core #4 (1 threads), logical processors [4]
  processor core #5 (1 threads), logical processors [5]
  processor core #6 (1 threads), logical processors [6]
  processor core #7 (1 threads), logical processors [7]
  capabilities: [sha1 sha2 crc32 cpuid
glimchb commented 1 year ago

@ffromani this could help with #199 and #303

ffromani commented 1 year ago

@jaypipes sure thing! I'll have a look and comment here ASAP. Thanks @glimchb to offer this

jaypipes commented 1 year ago

@jaypipes sure thing! I'll have a look and comment here ASAP. Thanks @glimchb to offer this

@ffromani grazie mille!

glimchb commented 1 year ago

@glimchb do you want to add some test cases that use this snapshot?

@jaypipes the CPU tests will fail until we merged #303 WDYT ? we can add other tests...

jaypipes commented 1 year ago

@jaypipes the CPU tests will fail until we merged #303 WDYT ? we can add other tests...

It's a shame that @rockrush did not want to respond to our PR review comments on that one. @glimchb what do you think about creating a PR that combines your snapshot addition with #303, addresses the PR reviews, and adds some simple unit tests for AARCH64 platforms?

ffromani commented 1 year ago

My 2c: the snapshot provided is surely a nice idea, and we will carefully evaluate it. I for myself would prefer a developer-accessible environment, because this seems an area on which we should expect some active development.

Unfortunately this is a bit of a busy period for me, but I'll get back and comment ASAP anyway.

EDIT: reportedly gh actions will add arm64 support "in the future" (https://github.com/actions/runner-images/issues/5631#issuecomment-1218092275). I'm seriously considering adding a workaround using QEMU (was also suggested in the past IIRC) as stopgap solution.

EDIT2: what I'm trying to say is: for amd64, we pretty much all have access to "basic" machines with the most common stuff. Using snapshot is great to test exotic configs or to prevent regressions. But as complement to having easy access to machines to develop. Depending only on snapshot (arm64) is suboptimal. Sure, it's a step forward, but we still have major maintainability concerns. Note this is my own take with my maintainer hat on, this does not represent the project vision.

glimchb commented 1 year ago

@glimchb what do you think about creating a PR that combines your snapshot addition with #303, addresses the PR reviews, and adds some simple unit tests for AARCH64 platforms?

Sure, I can do that for sure if we agree on the strategy - snapshot vs qemu or both... Another approach I used so far is run 1 single ci job for ARM64 on https://www.travis-ci.com/

glimchb commented 1 year ago

@ffromani not sure we can use QEMU for our use acse due to https://github.com/actions/runner-images/issues/183 I'm using a lot of qemu for multi platform docker builds but we need arm kernel not just instruction set emulation to see lscpu and /proc/cpuinfo correctly...

glimchb commented 1 year ago

@jaypipes @ffromani address all comments and broke to smaller commits, please review

glimchb commented 1 year ago

One more suggestion inline for you. Also, if you might edit your PR summary and put the following in there: I'm sure @rockrush would appreciate that!

Absolutely. Done, thanks for suggestion. The commit https://github.com/jaypipes/ghw/pull/333/commits/34f45f4cfdb628931eb1732fa414a7ae97191354 was cherry-picked, so it has proper Signed-off-by: Xingyou Chen <rockrush@rockwork.org> But as you suggested, also added this to PR description