jawn-smith commented 3 years ago

I decided not to add mocks for the apt-cache policy libc6 command in all test cases because the builds and autopkgtests should always be running in an environment with glibc > 2.33. If that is an unsafe assumption I can mock out those commands in all tests.

A sample of the command "ubuntu-report show" with the new version:

{ "Version": "21.04", "OEM": { "Vendor": "Micro-Star International Co., Ltd.", "Product": "MS-7C37", "Family": "To be filled by O.E.M." }, "BIOS": { "Vendor": "American Megatrends International, LLC.", "Version": "A.C0" }, "CPU": { "OpMode": "32-bit, 64-bit", "CPUs": "1", "Threads": "1", "Cores": "1", "Sockets": "1", "Vendor": "AuthenticAMD", "Family": "23", "Model": "113", "Stepping": "0", "Name": "AMD Ryzen 7 3700X 8-Core Processor", "Virtualization": "AMD-V" }, "Arch": "amd64", "HwCap": "x86-64-v3", "GPU": [ { "Vendor": "10de", "Model": "2484" } ], "RAM": 32.8, "Disks": [ 1000.2 ], "Partitions": [ 0.5, 0.5 ], "Screens": [ { "Size": "798mmx334mm", "Resolution": "2560x1080", "Frequency": "60.00" }, { "Size": "697mmx393mm", "Resolution": "2560x1440", "Frequency": "143.91" } ], "Autologin": false, "LivePatch": false, "Session": { "DE": "ubuntu:GNOME", "Name": "ubuntu", "Type": "x11" }, "Language": "en_US", "Timezone": "America/Chicago", "Install": { "Media": "Ubuntu 21.04 \"Hirsute Hippo\" - Alpha amd64 (20210123)", "Type": "GTK", "OEM": false, "PartitionMethod": "use_zfs", "DownloadUpdates": true, "Language": "en", "Minimal": false, "RestrictedAddons": true, "Stages": { "0": "language", "15": "console_setup", "17": "prepare", "64": "partman", "101": "partman", "111": "partman", "116": "start_install", "126": "timezone", "154": "usersetup", "173": "user_done", "523": "done" } } }

jawn-smith commented 3 years ago

Did not realize this would try to build on Xenial. I'll mock out the libc6 commands and make it compatible with the older io/ioutil library.

doko42 commented 3 years ago

I want to differentiate between "not available" and "no support for a specific hwcap". How would you do that instead?

doko42 commented 3 years ago

or better say "unable to determine" instead of "not available".

didrocks commented 3 years ago

I would prefer to reverse the logic so that we are in line with the other fields, meaning:

if the field is empty (not there) -> it means that we didn’t even try to collect the data and having it empty makes sense (older version of ubuntu-report, older releases with older glibc which can’t give us this data…)
having a special hardcoded string for "not available/unable to dermine" meaning that it’s an arch with a desired version of glibc (basically running hirsute+), but we know we can’t get those data before it’s not a supported arch.

Does it make sense?

doko42 commented 3 years ago

yes, we'll set this field to "-" for any check on glibc >= 2.33 where no specific hwcaps is supported.

codecov-io commented 3 years ago

Codecov Report

Merging #34 (495f327) into master (826661f) will increase coverage by 0.63%. The diff coverage is 94.23%.

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   82.84%   83.47%   +0.63%     
==========================================
  Files          12       12              
  Lines         886      938      +52     
==========================================
+ Hits          734      783      +49     
- Misses         88       90       +2     
- Partials       64       65       +1

Impacted Files	Coverage Δ
internal/metrics/testing.go	`0.00% <0.00%> (ø)`
internal/metrics/metrics.go	`95.04% <93.33%> (-0.57%)`	:arrow_down:
internal/metrics/cmd.go	`94.95% <100.00%> (+1.08%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 826661f...495f327. Read the comment docs.

jawn-smith commented 3 years ago

The existing interface for overriding the cmds is really only usable if that command is a member of the struct. I've kept libc6Cmd as a member of the Metrics struct but I'm not storing it there unless the override is created. So it shouldn't be stored in normal use, just in the test cases. The method for checking if the libc6Cmd is overridden by calling the functions passed to the constructor feels a little janky but it works. If there are any suggestions of a nicer way to do that I'd be happy to take a look.

I've also made the architecture->ld-linux map a local variable, made the switch to dpkg and changed the release to UNRELEASED

didrocks commented 3 years ago

LGTM! Let’s merge it and prepare a release :) I’ll let you upload it to ubuntu (with doko) once the FFe is approved.

You just need to gbp buildpackage -S from the git branch to automatically vendorize the content.

ubuntu / ubuntu-report

Include version 1.6.3 and add metric for hwcaps #34

Codecov Report