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

audit(github): prefer /archive/refs/tags urls over /archive #16126

Merged
merged 2 commits into from Oct 24, 2023

Conversation

chenrui333
Copy link
Member

@chenrui333 chenrui333 commented Oct 20, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

followup mislav/bump-homebrew-formula-action#53

seeing couple of issues recently in homebrew-core for the artifact urls

@Bo98
Copy link
Member

Bo98 commented Oct 21, 2023

Good idea.

As is, this won't work with commit URLs like with LuaJIT, but you should be able to adjust the regex to exclude those types of URLs.

@chenrui333
Copy link
Member Author

yeah, I will update all the urls later :)

@chenrui333
Copy link
Member Author

chenrui333 commented Oct 22, 2023

Added a big PR to fix all the url formats, Homebrew/homebrew-core#152092

passed my style check in my local brew style homebrew/core

@chenrui333
Copy link
Member Author

should be good to go

audit_urls(urls, archive_ref_tags_gh_pattern) do |_, url|
next if url.end_with?(".git")

problem "Use /archive/refs/tags URLs for GitHub tarballs (url is #{url})."
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to write an autocorrect for this if possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

any example of doing autocorrect?

Copy link
Member

Choose a reason for hiding this comment

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

corrector.replace(homepage_parameter_node.source_range, "\"#{homepage}/\"")

Can wait till a follow-up PR but, if it's not too hard: autocorrects are always nice to have.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Library/Homebrew/rubocops/urls.rb Show resolved Hide resolved
Signed-off-by: Rui Chen <rui@chenrui.dev>
@chenrui333
Copy link
Member Author

rebased to latest master after Homebrew/homebrew-core#152092 merge

@chenrui333
Copy link
Member Author

The current audit failures are due to the repo removal issues for hashpump and nethacked

/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hashpump.rb:4:3: C: FormulaAudit/Urls: Use /archive/refs/tags URLs for GitHub tarballs (url is https://github.com/bwall/HashPump/archive/v1.2.0.tar.gz).
  url "https://github.com/bwall/HashPump/archive/v1.2.0.tar.gz"
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/n/nethacked.rb:24:3: C: FormulaAudit/Urls: Use /archive/refs/tags URLs for GitHub tarballs (url is https://github.com/nethacked/nethacked/archive/1.0.tar.gz).
  url "https://github.com/nethacked/nethacked/archive/1.0.tar.gz"

@Bo98
Copy link
Member

Bo98 commented Oct 23, 2023

You will need to change them still. Can do it in separate PRs to make it obvious it's not tested (though the orginal URL is broken anyway so it doesn't matter much).

@chenrui333
Copy link
Member Author

I see, let me update those two then.

Signed-off-by: Rui Chen <rui@chenrui.dev>
@chenrui333
Copy link
Member Author

Should be good now.

@MikeMcQuaid
Copy link
Member

I'm happy to merge when @Bo98 is ✅

chenrui333 added a commit to chenrui333/azure-cli that referenced this pull request Oct 24, 2023
…tifact urls

relates to Homebrew/brew#16126

Signed-off-by: Rui Chen <rui@chenrui.dev>
@Bo98 Bo98 merged commit 141ac15 into Homebrew:master Oct 24, 2023
27 checks passed
@mahrud
Copy link
Contributor

mahrud commented Oct 25, 2023

This PR broke using a branch archive for head builds. What I mean is that this:

  head do
    url "https://github.com/Macaulay2/M2/archive/refs/heads/master.tar.gz"
  end

Now gives an error: (see for instance this build)

Formula/macaulay2.rb:24:5: C: FormulaAudit/Urls: Use /archive/refs/tags URLs for GitHub tarballs (url is https://github.com/Macaulay2/M2/archive/refs/heads/master.tar.gz).
    url "https://github.com/Macaulay2/M2/archive/refs/heads/master.tar.gz"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is only relevant for --HEAD builds, where I think it's important to allow getting the latest commits from a branch before they are tagged.

I think the original rationale for preferring archive/refs/tags urls makes sense, but since there can be no confusion with archive/refs/heads urls, I don't see a reason to disallow the latter.

@Bo98
Copy link
Member

Bo98 commented Oct 25, 2023

Yeah we should update the regex to exclude refs/heads - that makes sense to me.

@MikeMcQuaid
Copy link
Member

Yeah we should update the regex to exclude refs/heads - that makes sense to me.

@chenrui333 can you do this? Thanks!

@mahrud
Copy link
Contributor

mahrud commented Oct 28, 2023

I opened #16155 to address this for now.

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants