Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpuinfo (new formula) #171405

Closed
wants to merge 1 commit into from
Closed

cpuinfo (new formula) #171405

wants to merge 1 commit into from

Conversation

dlenski
Copy link
Contributor

@dlenski dlenski commented May 10, 2024

Adds thecpu-info, cache-info, and isa-info command-line tools from https://github.com/pytorch/cpuinfo, and the associated dynamic library and headers.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source cpuinfo
  • Is your test running fine brew test cpuinfo
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the new formula PR adds a new formula to Homebrew/homebrew-core label May 10, 2024
@@ -0,0 +1,39 @@
class Cpuinfo < Formula
Copy link
Contributor Author

@dlenski dlenski May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the CLI tool is named cpu-info, and that there is already a cask named cpuinfo, it might be better to name this cpu-info.

However, Debian calls it cpuinfo and so do other distros, so I'm genuinely unsure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me, cpuinfo sounds better than cpu-info.

Comment on lines 43 to 44
# TODO: test other arch-specific "dump" binaries, if present
# gpu-dump, auxv-dump, cpuid-dump, cpuinfo-dump
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's maybe remove it

Suggested change
# TODO: test other arch-specific "dump" binaries, if present
# gpu-dump, auxv-dump, cpuid-dump, cpuinfo-dump

class Cpuinfo < Formula
desc "CPU INFOrmation library and associated command-line tools"
homepage "https://github.com/pytorch/cpuinfo"
url "https://github.com/pytorch/cpuinfo/archive/3c8b1533ac03dd6531ab6e7b9245d488f13a82a5.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a tagged release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't exist 🤷‍♂️. Debian simply names the package versions gitYYYYMMDD.${GITHASH}. https://packages.debian.org/search?keywords=cpuinfo

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, @SMillerDev and @chenrui333, I see several other formulae where the "releases" are simply date-plus-Git-hash slugs.

the only catch is we need a tagged release. (as we only ship stable releases at best efforts)

Is this requirement a recently-added one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not a new requirement. luajit is a one-off exception, but there should not be any others. I'll do a review when I have time to confirm this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p-linnane, if I simply git grep -P -C5 'version "20\d\d' Formula/, I find numerous other formulae in which the version identifiers used by Homebrew are simply dates and/or Git hashes, with no sign of a clear versioning or tagging strategy that originates in the upstream projects.

Here is an example that appears nearly identical to this one:

url "https://github.com/douglascrockford/JSMin/archive/430bfe68dc0823d8c0f92c08d426e517cbc8de5e.tar.gz"
version "2019-10-30"
sha256 "24e3ad04979ace5d734e38b843f62f0dc832f94f5ba48642da31b4a33ccec9ac"
license "JSON"
# The GitHub repository doesn't contain any tags, so we have to check the
# date in the comment at the top of the `jsmin.c` file.
livecheck do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran that, and looked through the results. jsmin is the only one that matches what you are talking about here. All other results are us having to clarify the string we want to use as the version, because it can't be parsed from the filename. Additionally, it appears jsmin is the way it is because the repo has no tags, but the website states a release date, so that's what's being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsmin is the only one that matches what you are talking about here. All other results are us having to clarify the string we want to use as the version, because it can't be parsed from the filename.

How about, say, https://github.com/Homebrew/homebrew-core/blob/master/Formula/i/ipbt.rb#L3-L7?

The "release" is simply a tarball with a date and a Git hash embedded in it. The homepage simply mentions it as:

A Unix source archive of IPBT is available here:

ipbt-20240501.bc876ea.tar.gz

… without otherwise describing any strategy or approach to versioning.

I guess what I'd like to clarify here is: what is a minimum threshold for what counts as "versions that come from upstream"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That falls into the category I mentioned. It is a deliberate release, with the date being the version identifier as far as we are concerned. The point is that upstream for ipbt is distributing this version, so it's what we are using. Arbitrary commits are not enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex you want is probably something more like

git grep -P -C5 'url "https://github.com/[^/]*/[^/]*/archive/[0-9a-f]*\.tar\.gz"' -- Formula/

This produces a number of results, but most of the matches belong to resources (which don't need version tags). There are three formulae urls that match (i.e. the match doesn't belong to a resource): luajit, jsmin and solid.

Of those, solid and luajit used to provide tags/versioned download URLs (see the repo Git log for details). jsmin was likely added before we adopted the policy1 of only packaging software where upstream has version tags.

If you really want this in Homebrew/core, ask upstream if they're willing to tag their releases. Otherwise, you can host this formula in your own tap.

Footnotes

  1. https://docs.brew.sh/Acceptable-Formulae#stable-versions

@chenrui333
Copy link
Member

the only catch is we need a tagged release. (as we only ship stable releases at best efforts)

@chenrui333 chenrui333 added the pre-release Artifact is pre-release label May 11, 2024
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far this does not meet the requirements for inclusion: https://docs.brew.sh/Acceptable-Formulae#stable-versions

Adds the 'cpu-info', 'cache-info', and 'isa-info' command-line tools
from https://github.com/pytorch/cpuinfo, and the associated dynamic
library and headers.
@dlenski dlenski closed this May 12, 2024
@dlenski dlenski requested review from MikeMcQuaid and a team as code owners May 12, 2024 04:09
@github-actions github-actions bot added workflows PR modifies GitHub Actions workflow files automerge-skip `brew pr-automerge` will skip this pull request autobump and removed new formula PR adds a new formula to Homebrew/homebrew-core labels May 12, 2024
@dlenski
Copy link
Contributor Author

dlenski commented May 13, 2024

@SMillerDev wrote:

So far this does not meet the requirements for inclusion: https://docs.brew.sh/Acceptable-Formulae#stable-versions

As I show in #171405 (comment), there are many counter-examples to this, where there are previously accepted formulae in homebrew-core that lack any sign of an upstream versioning strategy, and where the version identifiers maintained by Homebrew are simply dates/timestamps and/or Git hashes.

I definitely do completely understand the desire to use and rely on coherent upstream versioning wherever possible, though 😅. And thank you @chenrui333 for pinging pytorch/cpuinfo#84 (comment).

Since there are numerous other free-software distributions which manage to stably package https://github.com/pytorch/cpuinfo, what would you think of piggybacking on the specific versions that they package? My preference would be to pick the versions packaged by Debian, as a very well-organized and relied-upon Linux distribution.

@p-linnane
Copy link
Member

As I show in #171405 (comment), there are many counter-examples to this, where there are previously accepted formulae in homebrew-core that lack any sign of an upstream versioning strategy, and where the version identifiers maintained by Homebrew are simply dates/timestamps and/or Git hashes.

Please see my response to the comment in question, because this is not an accurate statement.

@MikeMcQuaid
Copy link
Member

Also: new formulae have a higher bar to reach than existing, widely used formulae.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autobump automerge-skip `brew pr-automerge` will skip this pull request pre-release Artifact is pre-release workflows PR modifies GitHub Actions workflow files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants