Repository navigation
Add analytics reporting for the x86 version - #24054
SMillerDev wants to merge 1 commit into
Conversation
|
Passing on this given conversation in the discussion. |
|
I've read it again, but I only see argument there in favour of tracking this information. |
|
@SMillerDev TL;DR: I don't see a world in which we change this and do a mass rebottling for something without significant performance benchmarks, unless we're getting a hard 0️⃣ here. As-is this will mess with our existing analytics here too. I just don't think it's worth the large amount of effort here and breakage. |
|
I do see a possible future where we add a separate bottle for This is the direction that major Linux distros have been preparing recently so users may start seeing a performance loss if switching from system to Homebrew binaries:
|
|
@cho-m Thanks for context. I think logical time for this sort of change for us might be when we're doing Ubuntu/GCC/glibc version migrations. We could start using different bottle tags for each of those and consider adjusting what we support for Intel/ARM at those points. |
3887d4a to
690f08d
Compare
690f08d to
90bc603
Compare
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title correctly identifies the addition of analytics reporting, which is part of the change. However, the implementation reports ARM CPU family data rather than an x86 version, so the architecture… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Title check
-
Explanation
The title correctly identifies the addition of analytics reporting, which is part of the change. However, the implementation reports ARM CPU family data rather than an x86 version, so the architecture description is inaccurate.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
90bc603 to
ccb4ca6
Compare
9e15d70 to
279c283
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
607f29f1-f270-4204-8f8d-c298f80d1268
📒 Files selected for processing: 3
Library/Homebrew/extend/os/linux/hardware/cpu.rbLibrary/Homebrew/test/utils/analytics_spec.rbLibrary/Homebrew/utils/analytics.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
|
Output from my RPI now: HOMEBREW_VERSION: 7.0.9-161-g5ab0620
ORIGIN: https://github.com/Homebrew/brew
HEAD: 5ab06207b34e09426ad0ca94de7110c577b915fb
Last commit: 3 minutes ago
Branch: feat/analytics/report_x86_version
Core tap: N/A
Core cask tap: N/A
HOMEBREW_PREFIX: /home/linuxbrew/.linuxbrew
Homebrew Ruby: 4.0.7 => /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/vendor/portable-ruby/4.0.7_1/bin/ruby
CPU: quad-core 64-bit armv8_0_a |
There was a problem hiding this comment.
🧹 Nitpick comments: 1
Library/Homebrew/test/utils/analytics_spec.rb: 1
38-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep the existing custom-prefix example cross-platform and add a Linux consumer example.
:needs_linuxwould skip the existing example on macOS. Keep that example unchanged. Add a separate Linux-only example that supplies ARMcpuinfodata and asserts the serialised analytics value. A fixture used only byarm_familytests would not testdefault_package_tags.Suggested fix
it "returns OS_VERSION, ARM and prefix when HOMEBREW_PREFIX is a custom prefix on arm" do expect(Homebrew).to receive(:default_prefix?).and_return(false).at_least(:once) expect(described_class.default_package_tags).to have_key(:cpu_family) expect(described_class.default_package_tags).to have_key(:arch) expect(described_class.default_package_tags[:arch]).to eq HOMEBREW_PHYSICAL_PROCESSOR expect(described_class.default_package_tags).to have_key(:prefix) expect(described_class.default_package_tags[:prefix]).to eq "custom-prefix" end + it "serialises the Linux ARM CPU family in the package tags", :needs_linux do + allow(Hardware::CPU).to receive_messages(arm?: true, intel?: false) + allow(Hardware::CPU).to receive(:cpuinfo).and_return( + "CPU implementer : 0x41\nCPU part : 0xd08\n", + ) + described_class.clear_cache + + expect(described_class.default_package_tags[:cpu_family]).to eq "armv8_0_a" + end +
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
84644939-885c-4d00-aaac-221244b44c38
📒 Files selected for processing: 1
Library/Homebrew/extend/os/linux/hardware/cpu.rb
🚧 Files skipped from review as they are similar to previous changes: 1
- Library/Homebrew/extend/os/linux/hardware/cpu.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
5ab0620 to
ca12a5d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve the PowerPC family value before the unknown fallback. · cpu.rb:43-47
Library/Homebrew/extend/os/linux/hardware/cpu.rb:43-47
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the PowerPC family value before the unknown fallback.
A supported Linux PowerPC host reaches this branch and now reports
:dunno. This changes thecpu_familypackage analytics value and the 32-bit PowerPC installation metadata fromppctodunno. Add a PowerPC branch so the new ARM unknown-family behaviour remains unchanged.Suggested fix
- else + elsif ppc? + :ppc + else :dunno
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
39f16fb7-444f-4e51-a195-289dba48f543
📒 Files selected for processing: 1
Library/Homebrew/test/utils/analytics_spec.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
ca12a5d to
8266383
Compare
This branch has not been deployed
3 participants
brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Inspired by https://github.com/orgs/Homebrew/discussions/7092#discussioncomment-18528421, this now reports if the x86 processor has v1-4 support. That would be good to know to potentially make bottles on newer x86 versions saving developers time and the world energy by using more optimized instructions.
Summary by CodeRabbit