brew audit should validate head urls exist

[ilovezfs]

At least for GitHub head urls I've found this test detects failures:

if foo_head_url =~ /(.*).git.?$/
  return if `curl -s "#{$~[1]}"`.include?("error\":\"Not Found")
end

but perhaps there's a more general solution like directly attempting a clone and aborting on success, or returning failure if it times out or there's an error.

[MikeMcQuaid]

I was thinking the other day about introducing head builds to CI but I'm unsure what I think about that.

[ilovezfs]

I only noticed this was an issue because it causes brew livecheck to stop in its tracks and prompt for username and password when non-existent GitHub head urls are encountered, which the test above works around, but which, of course, also adds to the total run time.

[ilovezfs]

By the way that's also the behavior that our end users get when they attempt to brew install --HEAD foo with a non-existent GitHub head url:

iMac-TMP:~ joe$ brew install --HEAD aws-sdk-cpp
==> Cloning https://github.com/awss/aws-sdk-cpp.git
Updating /Users/joe/Library/Caches/Homebrew/aws-sdk-cpp--git
Username for 'https://github.com': 
Password for 'https://github.com': 
remote: Repository not found.
fatal: Authentication failed for 'https://github.com/awss/aws-sdk-cpp.git/'
Error: Failed to download resource "aws-sdk-cpp"
Failure while executing: git fetch -q origin

[ilovezfs]

We could also use the curl workaround above to detect the issue just-in-time and report out a better error, and then wait for a user to open an issue about it, but audit seems a little more friendly. I guess we could also do both since there's no guarantee the url is still valid even though it passed audit during the last CI whenever that may have been (for example, memo which went entirely MIA).

[ilovezfs]

Regarding head builds for CI, I wonder if that would make sense as something to run asynchronously when CI resources are idle?

[DomT4]

I was thinking the other day about introducing head builds to CI but I'm unsure what I think about that.

I've considered this a few times and ended up at resounding "Nope" every time in my head. I'm not sure we have the capacity to handle it, a PR for a formula could pass Monday morning but fail Monday evening for reasons wholly unrelated to the actual change, and CI testing implies supporting upstream heads & fixing where necessary, which makes me nervous.

[MikeMcQuaid]

CI testing implies supporting upstream heads & fixing where necessary, which makes me nervous.

I kinda think the opposite; it implies removing broken upstream heads but maybe @ilovezfs's method is enough for that.

[DomT4]

My worry there is similar. It breaks Monday so I commit a removal of head, someone files a PR on Thursday adding a head line back and it's merged by another maintainer because none of us have the time to go hunting through formula histories to review every single PR, and then it breaks again Sunday.

HEADs are just inherently unreliable and I think it does reduce our support burden to simply be able to turn around and say "We don't spend time/resources supporting head, install it at your own risk".

I'm in favour of a simple curl -I -L ding on heads to check they still exist in the audit, but somewhat problematically that would seem to mandate audit can't run offline, or we tag the check with @online which means it doesn't get run regularly.

[MikeMcQuaid]

I'm in favour of a simple curl -I -L ding on heads to check they still exist in the audit, but somewhat problematically that would seem to mandate audit can't run offline, or we tag the check with @online which means it doesn't get run regularly.

@DomT4 I think it's fine to have it be part of --online.

HEADs are just inherently unreliable and I think it does reduce our support burden to simply be able to turn around and say "We don't spend time/resources supporting head, install it at your own risk".

Feature request: print this message before attempting HEAD installation.

[ilovezfs]

or we tag the check with @online which means it doesn't get run regularly.

Why is that?

[DomT4]

Feature request: print this message before attempting HEAD installation.

Should be simple enough. I'll see if I can squeak a quick PR later tonight, working on other things at the moment 😅.

Why is that?

It contains things that we don't want run every time, like GitHub notability checks, which we don't "care" about if we've already accepted the formula.

[MikeMcQuaid]

It contains things that we don't want run every time, like GitHub notability checks, which we don't "care" about if we've already accepted the formula.

Well, to be more specific, it contains things that require an internet connection to check.

[DomT4]

Oh, yeah, that too 🙈. You're supposed to be able to run audit offline if desired. I need more coffee, apparently.

[ilovezfs]

It looks like test-bot uses --online so I'm wondering in what sense you meant "not regularly." Are you referring to how often it's run? (If so, running every time it goes through test-bot seems regularly enough.) Or do you just mean not regularly as in "the vanilla invocation without additional command line switches"?

[DomT4]

test-bot only uses --online when the PR is handling a new formula. Otherwise a vanilla brew audit is executed instead.

[ilovezfs]

Ah, that's a shame.

[DomT4]

It's possible we should further break down --online into --online and --new-submission but we had enough trouble getting people to start using --online regularly so I'm not sure how much love anyone would have for the idea.

[MikeMcQuaid]

There's --online and --strict; I think the latter implies new submission and we should run brew audit --online in the bot 👍

[DomT4]

We'd probably want to tweak audit_github_repository to make sure it only runs when both --strict and --online are passed then?

[ilovezfs]

I wonder if this should be handled via in-formula DSL instead of command line options.
Like

indulgences => [:notability, :test]

No, I'm not actually suggesting DSL literally named indulgences. ;)

[MikeMcQuaid]

We'd probably want to tweak audit_github_repository to make sure it only runs when both --strict and --online are passed then?

@DomT4 Yeh, probably. Definitely 👎 on having this be a DSL in formulae themselves; like brew doctor the point of (most of) these warnings is to flag badly written formulae and we shouldn't really allow an opt-out. There are some what may perhaps only apply to official taps, though.

[lwe]

Checking the latest output of a --HEAD install it looks like the message (_We don't spend time/resources supporting head, install it at your own risk_) is not yet implemented and I assume the same is true for audit_head_repository on --strict and --online.

If yes and these two changes are still wanted, I'd like to pick this up as these look like a good starter items for me.

• Print the message somewhere in FormulaInstaller
• Extend brew-audit --strict --online to check GH head URLs, maybe extend to git/svn head URLs too?

[MikeMcQuaid]

Extend brew-audit --strict --online to check GH head URLs, maybe extend to git/svn head URLs too?

Hey @lwe! I'd just do this; the other previous ideas aren't really useful as-is. There's been recent changes to audit.rb that should make it relatively trivial to check HEAD URL validity.

[lwe]

Maybe, it's just me... but with the new ResourceAuditor from 9fa01471 this seems already been taken care of. After I changed the head url in the_silver_searcher.rb formula, audit reported the following:

brew audit ag
# => 0

brew audit --strict ag
# => 0

brew audit --online ag
# the_silver_searcher:
#  * HEAD: The URL https://github.com/ggreer/the_silver_searcherx.git is not a valid git URL
# Error: 1 problem in 1 formula
# => 1

As brew head URLs already seem to be properly checked, even using a proper download strategy (for curl, git, svn) I suggest to close this issue and mark as resolved.

_Update_: except that it's running for --online only and does not require both --online --strict args, but I guess this is probably okay?

[ilovezfs]

Yeah this was already done.