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

lsusb 1.1.4 #163135

Closed
wants to merge 1 commit into from
Closed

lsusb 1.1.4 #163135

wants to merge 1 commit into from

Conversation

wyan
Copy link

@wyan wyan commented Feb 18, 2024

  • 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 <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • 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>?

The previous version of this formula was pointing to a repo that is now abandoned, I have updated the URL to the most current fork of this project.

@github-actions github-actions bot added macos-only Formula depends on macOS automerge-skip `brew pr-automerge` will skip this pull request labels Feb 18, 2024
@github-actions github-actions bot removed the automerge-skip `brew pr-automerge` will skip this pull request label Feb 18, 2024
@iMichka
Copy link
Member

iMichka commented Feb 18, 2024

Thanks for your contribution.
For reference: some discussions about the best fork to chose happened here: jlhonora/lsusb#19

The proposed fork has only 4 stars, so that does not really meet our notability criteria.
Maybe the original author could endorse one of the forks?

I'm also worried that there are so many downloads for a seemingly abandoned project ...

==> Analytics
install: 783 (30 days), 2,278 (90 days), 7,601 (365 days)
install-on-request: 783 (30 days), 2,278 (90 days), 7,601 (365 days)
build-error: 0 (30 days)

@iMichka iMichka added the automerge-skip `brew pr-automerge` will skip this pull request label Feb 18, 2024
@wyan
Copy link
Author

wyan commented Feb 18, 2024

Thank you for your comment, that is indeed the discussion from which I learnt about the proposed fork, as the version currently in homebrew-core has a bug that, while not serious, is annoying. If the proposed PR does not meet notability criteria, perhaps it'd be convenient to consider removing the formula (I'm happy to move it to a personal tap), but I'm not familiar with the procedure to do so.

@iMichka
Copy link
Member

iMichka commented Feb 18, 2024

What about usbutils? Does it provide equivalent functionality? Is it well maintained? Maybe we could drop the formula and recreate users to usbutils?

I'm worried to deprecate the formula as there are 783 downloads / month. We will not advertise private taps so we can't redirect our users to your tap. So that's also not an ideal solution.

@wyan
Copy link
Author

wyan commented Feb 18, 2024

The lsusb command in usbutils doesn't seem to work properly on my machine, sadly.

The version currently in homebrew-core sort of works, but displays the following error:
/opt/homebrew/bin/lsusb: line 89: 16#: invalid integer constant (error token is "16#")
once per USB device found. The fix should be really brief, but the original repo has several PRs pending, the oldest since 2021, so I'm not sure it'd be easy to get them to endorse any alternative repos.

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

LGTM, I think it would be useful for some folks! 👍

@chenrui333 chenrui333 added the ready to merge PR can be merged once CI is green label Feb 24, 2024
@iMichka
Copy link
Member

iMichka commented Feb 24, 2024

I would like some more thoughts from @Homebrew/core maintainers. Looks good IMHO, let's just wait a few more days if someone wants to block this from being merged.

@p-linnane
Copy link
Member

@LanikSJ's fork is the most active, and people obviously care enough so let's merge it.. They've also been a Homebrew contributor for some time, so that helps me with my decision 😆.

@LanikSJ
Copy link
Contributor

LanikSJ commented Feb 24, 2024

Thank you @p-linnane I fully intend to support this project as long as humanly possible.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Hold please. Usual/documented fork rules should apply: either upstream needs to bless this fork or we need evidence that multiple other distros have moved to this fork.

I've skimmed for evidence of either and can't see them. That's not saying they aren't the case, just let's be explicit in the PR first, thanks.

@Bo98
Copy link
Member

Bo98 commented Feb 25, 2024

we need evidence that multiple other distros have moved to this fork

On this point it's worth noting we're the only known package manager to ship this at all, largely because it's macOS-only. So there are no other distros to check.

@MikeMcQuaid
Copy link
Member

On this point it's worth noting we're the only known package manager to ship this at all, largely because it's macOS-only. So there are no other distros to check.

In that case: I'd rather vote (again, as per-docs) that we disabled it rather than move to a fork that doesn't seem to be sufficiently canonical. We could also consider accepting this as-is as lsusb-LanikSJ if other notability guidelines are met.

@LanikSJ
Copy link
Contributor

LanikSJ commented Feb 26, 2024

I'm willing to take whatever steps necessary for this formula to be included. I haven't proposed it myself as in my experience I didn't think it would get included.

In the mean time I maintain my own tap https://github.com/LanikSJ/homebrew-tap however I don't pretend to know how write Taps correctly. 🙂

@p-linnane
Copy link
Member

I've opened #164273 to add @LanikSJ's fork as a new formula. Will deprecate this one once that's merged.

@p-linnane p-linnane removed the ready to merge PR can be merged once CI is green label Feb 26, 2024
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request macos-only Formula depends on macOS outdated PR was locked due to age repo-location-update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants