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

Tell curl to not use .curlrc #4022

Merged
merged 3 commits into from Apr 8, 2018

Conversation

Projects
None yet
3 participants
@stefansundin
Copy link
Contributor

stefansundin commented Apr 6, 2018

Searching the issues and PRs, I wasn't able to find prior discussion of this. Apologies if this is something that has already been decided that it can't be used for some reason.

I have known that Homebrew does not like ~/.curlrc because brew doctor warns about it. But it hasn't been an issue for me until I tried creating a cask (Homebrew/homebrew-cask#45612).

In deciding between -q and --disable, I chose -q because it appears that --disable was only added in 7.49.0 (May 18 2016). I wasn't able to determine when -q was initially added.

One reason I can think about against not merging this, is if a user puts special proxy settings in their curlrc. But even in that case I'm not sure, because the user might not expect Homebrew to use that. Is there a way to tell curl to use the macos proxy settings?

Let me know what you think! Thanks!


  • 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 tests with your changes locally?
@reitermarkus
Copy link
Member

reitermarkus left a comment

One reason I can think about against not merging this, is if a user puts special proxy settings in their curlrc.

There is a whole section with environment variables in the man page which can be used to set the proxy used by curl, so I don't think this is a problem.

describe "curl" do
describe "curl_args" do
it "returns -q as the first argument" do
expect(curl_args("foo")[1]).to eq("-q")

This comment has been minimized.

@reitermarkus

reitermarkus Apr 6, 2018

Member

expect(curl_args("foo")).to include("-q")

This comment has been minimized.

@stefansundin

stefansundin Apr 6, 2018

Contributor

-q has to be the first argument.

From https://curl.haxx.se/docs/manpage.html:

-q, --disable
If used as the first parameter on the command line, the curlrc config file will not be read and used.

This comment has been minimized.

@reitermarkus

reitermarkus Apr 6, 2018

Member

Ah, ok, fine as is then.

@stefansundin stefansundin force-pushed the stefansundin:disable-curlrc branch from 77e99c2 to 4875dc8 Apr 6, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 6, 2018

Thanks for the PR!

One reason I can think about against not merging this, is if a user puts special proxy settings in their curlrc.

Perhaps we could have a HOMEBREW_CURLRC variable or similar that tells Homebrew (not by default) to use the system .curlrc? We could document that under both the Environment Variables and Proxy settings of the manpage.

CC @ilovezfs for thoughts on this PR and the above given we've discussed this before.

describe "curl" do
describe "curl_args" do
it "returns -q as the first argument" do
# -q must be the first argument, or else it does not work

This comment has been minimized.

@reitermarkus

reitermarkus Apr 6, 2018

Member

Replace “or else it does not work” with ”according to man curl”.

@stefansundin stefansundin force-pushed the stefansundin:disable-curlrc branch from 4875dc8 to 0ec4781 Apr 6, 2018

@stefansundin

This comment has been minimized.

Copy link
Contributor

stefansundin commented Apr 6, 2018

Let me know if you want me to remove check_user_curlrc from diagnostics.rb.

@MikeMcQuaid MikeMcQuaid force-pushed the stefansundin:disable-curlrc branch from 2532f49 to d331c47 Apr 8, 2018

@MikeMcQuaid MikeMcQuaid merged commit 99b89e4 into Homebrew:master Apr 8, 2018

3 checks passed

codecov/patch 100% of diff hit (target 70.02%)
Details
codecov/project 70.03% (+0.01%) compared to 54a594e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
args = [curl_executable.to_s]

# do not load .curlrc unless requested (must be the first argument)
if ENV["HOMEBREW_CURLRC"]

This comment has been minimized.

@stefansundin

stefansundin Apr 9, 2018

Contributor

Isn't this backwards? Now we will still load curlrc by default.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 9, 2018

Member

@stefansundin It is indeed, I'm a moron! Fixed in #4045, thanks for noticing!

@lock lock bot added the outdated label May 9, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 9, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.