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
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Linux CPU family detection Library/Homebrew/extend/os/linux/hardware/cpu.rb, Library/Homebrew/test/hardware/cpu_spec.rb |
ARM detection maps implementer and CPU part values to architecture-family symbols. Other implementers and unmapped models return unknown symbols. PPC detection is removed, and other unsupported CPU types return :dunno. The CPU family test list includes seven ARM family symbols and accepts unknown family symbols. |
CPU family analytics tag Library/Homebrew/utils/analytics.rb, Library/Homebrew/test/utils/analytics_spec.rb |
default_package_tags adds the CPU family as a string-valued tag. The ARM custom-prefix test checks the :cpu_family tag and verifies :arch. A test checks that an ARM CPU produces the "armv8_0_a" tag. |
Priority:
Merge Risk: 
0053e
-
No actionable CPU-family or analytics risk remains for supported Homebrew workflows.
Security architecture risk: 
04757
-
This adds CPU-family information to package telemetry without changing its destination or opt-out controls. The reviewed changes show no new privilege or access path. Downstream handling of the new telemetry field was not available for review.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The direct security-relevant exposure is additional host classification data in package analytics for clients whose existing analytics checks permit reporting. The change does not select a new destination or grant authority to the CPU-family value.
Trust Boundaries and Controls
- observed — Both package-event reporting and final submission retain the existing analytics disable and per-run suppression checks. Tag serialization escapes spaces, commas, and equals signs; submission passes the body as a process argument rather than interpolating it into a shell command. These controls predate the PR and remain in place for the added field.
Resilience and Maintainability Implications
- inferred — The new field uses the existing lazy tag cache and event-specific hash merge. It introduces no new durable state transition or recovery protocol; event construction does not overwrite cached tags with event-specific values. Transport timeout and detached submission behavior are unchanged.
-
Pre-merge checks: 
-
Check name Status Explanation Title check The title identifies the analytics reporting change and the x86 focus described in the objectives. It is related to the changeset, although it does not mention the broader CPU family detection and ARM… Description check The description explains the purpose of reporting x86 processor support levels and relates directly to the analytics changes in the pull request. 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 identifies the analytics reporting change and the x86 focus described in the objectives. It is related to the changeset, although it does not mention the broader CPU family detection and ARM updates.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
90bc603 to
ccb4ca6
Compare
9e15d70 to
279c283
Compare
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
04757e8 to
21c7166
Compare
|
Well that's an interesting failure: https://github.com/Homebrew/brew/actions/runs/40104299741/job/120902914789?pr=24054 |
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
11694d09-d009-4fe6-8dad-da00e1d53785
-
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.
-
21c7166 to
a303041
Compare
some GitHub downtime/issues I guess, seeing that on mine too |
a303041 to
935c92b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b307c1ea-91c4-4b33-abe3-ed574804377d
-
Library/Homebrew/extend/os/linux/hardware/cpu.rbLibrary/Homebrew/test/hardware/cpu_spec.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.
-
935c92b to
58bc2e8
Compare
58bc2e8 to
0053e23
Compare
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.