Homebrew should make use of .curlrc #15419

Closed
wants to merge 1 commit into from

4 participants

@sfnelson

Rather than silently ignoring it (#13836).

Includes a doctor check to detect .curlrc and ensure that http downloads work.

(accidentally closed previous pull request (#15383))

@samueljohn

I like this doctor check because it tests general connectivity. Also good that it mentions the connection might be offline. The good thing is people can't open issues about this while being offline :-D

Hopefully, for difficult internet connection setups, people can fiddle with their .curlrc to make it work through proxies and such.

You removed the -q from the curl-flags. What does it do?

Besides: You'll need to squash into a single commit. Do you know how to do that with git rebase -i HEAD~3?

@sfnelson

The -q flag causes curl to ignore a user's curlrc configuration file (see #13836). I did not squash these commits because I've already had comments on them from my previous pull request (#15383). I wanted those who commented to be able to see what has changed. I can squash them if required.

I did wonder whether this doctor check should be applied whether or not the user has a custom .curlrc file: checking internet connectivity seems like a good idea in general. The check could augment the message with a comment about .curlrc if the check fails and the file exists. Thoughts?

@sfnelson sfnelson closed this Oct 13, 2012
@sfnelson sfnelson reopened this Oct 13, 2012
@jacknagel jacknagel commented on an outdated diff Oct 15, 2012
Library/Homebrew/cmd/doctor.rb
+ end
+ return
+ rescue
+ end
+
+ <<-EOS.undent
+ Homebrew was unable to access 'http://github.com' using curl
+ Homebrew needs curl to download packages. If your internet connection
+ is working otherwise it is possible that your curl configuration file
+ (~/.curlrc) is at fault.
+
+ Please make sure that you can run the following command successfully:
+ curl http://github.com
+ EOS
+end
+

The structure here can be simplified a bit by moving the rescue to the method level and sticking the heredoc inside:

def check_user_curlrc
  return if File.exist? "#{ENV['HOME']}/.curlrc"

  nostdout { curl('-o', '/dev/null', '--head', 'http://github.com') }
rescue
  <<-EOS.undent
    ...
  EOS
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacknagel

I think the scope of the check is fine for now; I can see the argument for a blanket "can we download things" check, but it does mean that the user wouldn't be able to get a clean bill of health from brew doctor when offline, so...

@sfnelson

Thanks for the suggestions. I'm not very familiar with ruby and I didn't realise I could do that. I've made the changes you suggest.

Given that my doctor check is now 2 functional lines (written by jacknagel), and the whole point of the commit is to remove a flag that should never have been set in the first place... time to pull?

@mxcl
Homebrew member

I'm removing the curl test, but leaving the warning if .curlrc exists. This is more in line with how brew doctor works generally (ie. diagnose but don't do full testing for side-effects), and removes my issue with how this patch works.

@mxcl mxcl added a commit that closed this pull request Oct 28, 2012
@sfnelson sfnelson Homebrew should not ignore curlrc
Added doctor check for .curlrc rather than silently ignoring it (#13836).

Closes #15419.

Signed-off-by: Max Howell <mxcl@me.com>

Removed test in doctor where it actually curl'd a file. It's enough to warn if the curlrc exists. I understand people want to remove the warnings, but the point in the doctor is to help diagnose and not to be some ramification of your UNIX system.
ef25978
@mxcl mxcl closed this in ef25978 Oct 28, 2012
@Sharpie Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Nov 4, 2012
@sfnelson sfnelson Homebrew should not ignore curlrc
Added doctor check for .curlrc rather than silently ignoring it (#13836).

Closes #15419.

Signed-off-by: Max Howell <mxcl@me.com>

Removed test in doctor where it actually curl'd a file. It's enough to warn if the curlrc exists. I understand people want to remove the warnings, but the point in the doctor is to help diagnose and not to be some ramification of your UNIX system.
183d3c3
@snakeyroc3 snakeyroc3 pushed a commit to snakeyroc3/homebrew that referenced this pull request Dec 17, 2012
@sfnelson sfnelson Homebrew should not ignore curlrc
Added doctor check for .curlrc rather than silently ignoring it (#13836).

Closes #15419.

Signed-off-by: Max Howell <mxcl@me.com>

Removed test in doctor where it actually curl'd a file. It's enough to warn if the curlrc exists. I understand people want to remove the warnings, but the point in the doctor is to help diagnose and not to be some ramification of your UNIX system.
0c922ee
@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.