Add proxy support to bootstrap and rebar3. Enhancement #561.

[carlosedp]

Add proxy support to bootstrap and rebar3.

Either bootstrap and rebar3 now uses environment variables http_proxy and https_proxy to fetch dependency packages and hex plugins.

Enhancement #561.

[tsloughter]

It shouldn't be os vars. In the plugin they are read from the hex config, https://github.com/hexpm/rebar3_hex/blob/master/src/rebar3_hex_config.erl#L97

We either should always look in that file or we can support them being set in ~/.config/rebar3/rebar.config as well as the hex config.

[carlosedp]

On rebar3 I believe reading from ~/.config/rebar3/rebar.config is fine but on bootstrap I think it would be better to read OS default since rebar is not even installed in the machine.
Isn't better to keep the settings on one file (rebar) instead of hex config?

[tsloughter]

I don't like the use of os vars, even in bootstrap. But I'd be fine with bootstrap looking in ./rebar.config or taking the proxy as an argument. What do you think of one of those options?

In rebar3.erl it should definitely use ~/.config/rebar3/rebar.config or hex.config. The reason we have to support hex's settings is they can be shared between mix and rebar3. Since the hex config has to be supported I figured it would simplify things by only supporting that one, instead of having competing config files for these settings.

Also, can you simplify the logic by copying how it is done in rebar3_hex, https://github.com/hexpm/rebar3_hex/blob/master/src/rebar3_hex_http.erl#L107-L115

[carlosedp]

What about first check ~/.config/rebar3/rebar.config for the vars. If not present check OS. I would implement on both bootstrap and rebar3. No need to change rebar3 rebar.config. The person building it would need to add the vars to it's own ~/.config/rebar3/rebar.config.

[tsloughter]

I still don't think we should ever look at any OS vars. We don't use them for anything besides DEBUG, it feels out of place.

[carlosedp]

The way I left is that the user must configure proxy vars on both ~/.config/rebar3/rebar.config and ~/.hex/hex.config. Is this the desired way to have it?
Having it all on rebar.config is against the need to have the config shared with mix but having it only on hex.config is not intuitive since rebar have it's own config. Any direction?

[tsloughter]

Looking good. But you should use rebar:dir:global_config/1 to get the config in utils. I think after that change and tests pass this will be good to merge.

[carlosedp]

Sorry about that.. I don't know the rebar API very well. Found the problem with the failing tests. The "hex" profile on httpc is not needed anymore. Fixed the test cases and will push in a couple minutes.
Thanks!

[tsloughter]

Oh right, can you actually change that to using a rebar profile for lhttpc and set options on that?

[carlosedp]

You mean replacing the "hex" profile for "rebar" on httpc calls? No problem.
One thing tho, on bootstrap I need to read the rebar.config directly since there is no rebar_dir module available. Ok?

[tsloughter]

Yup, replacing hex with rebar in httpc calls.

And yea, in bootstrap that is fine.

[ferd]

Also whatever happens, this stuff needs tests.

[tsloughter]

@ferd how would you test this with out either setting up a proxy for the test or mocking internal parts of httpc?

[ferd]

At the very least, we should trust httpc to be fine in its proxy usage if the configuration is passed in correctly. So the test I have in mind would check the different configuration values we want to support, and that if that's the case, they are properly reflected in the profiles and options we pass to httpc.

[tsloughter]

Ah, wait, duh, in a test it can just use httpc:get_options on the rebar profile before or after a run to check that the proxy values are correct.

[carlosedp]

Any news when this will be merged?

[tsloughter]

It looks good, but could you add a simple test, probably in rebar_deps_SUITE, that adds proxy options to the config and checks with httpc:get_options that after running with run_and_check it has the expected proxy settings? If the tests are confusing I may be able to get to writing the test later today.

[tsloughter]

@ferd no, you can't do that in bootstrap.

[ferd]

Yeah I just realized and tried to flush my comments. Saw it was fine later in the actual file.

[carlosedp]

I'm having problems with the tests. The scenario gets the variable from my own configured file and not the values injected into the mock config:

I created them in rebar_pkg_SUITE since I saw similar httpc tests on it:

init_per_testcase(proxy_settings=Name, Config0) ->
    Pkg = {<<"goodpkg">>, <<"1.0.0">>},
    Config1 = [{good_cache, false},
               {pkg, Pkg},
               {http_proxy, "http://localhost:1234"},
               {https_proxy, "http://localhost:1234"}
              | Config0],
    Config = mock_config(Name, Config1),
    meck:unload(httpc),
    meck:new(httpc, [passthrough, unsticky]),
    meck:expect(httpc, request, fun(_, _, _, _, _) -> {error, econnrefused} end),
    Config.

But when I check the result, it gets my rebar.config values instead of the values above:

proxy_settings(_Config) ->
    ?assertEqual({ok,{{"localhost", 1234}, []}},
                 httpc:get_option(proxy, rebar)).

Can you help me out?

[ferd]

The mock_config function mocks hex indexes and whatnot rather than a rebar configuration.

What you probably want to mock instead will be for rebar_dir:global_config() to return something like Common Test's priv_dir value, where you can put your own fake rebar3 configuration file. It's from that file that the config will be read.

You can then have your test call rebar_utils:set_httpc_options() and make sure everything is set right.

As part of the end_per_testcase/2 call, you should unmock the global config value and set the httpc options again so that everything runs under its original context for the rest of the run.

[carlosedp]

Oops.. I don't know why the robocopy fix went into this pull request. Need to remove it.

[carlosedp]

That's it. Fixed the branch (removed the two wrong commits).
I believe its correct now.
Thanks and sorry for bothering you guys!

[carlosedp]

The tests are failing on R15B03 because httpc module did not support https_proxy, only proxy.
How to manage this?

[ferd]

I'm guessing we'd have to detect in init_per_testcase whether the OTP version is R15Bxx (or prior) and if so, return {skip, https_proxy_unsupported_before_R16} or something, which should not be perceived as a failure.

[carlosedp]

Don't know why Travis still fails on R15 even when checking if OTP Vsn is =<15.
Here on my machine it correcly tests when I keep "15" and skips if I change the case to "18".
My OTP is Vsn 18.

[ferd]

@carlosedp cherry-pick this commit and it will work: ferd@74c6847

[carlosedp]

Thanks @ferd. It's passing now.

[c0b]

I still don't think we should ever look at any OS vars. We don't use them for anything besides DEBUG, it feels out of place.

Could you explain why not honoring os env var http_proxy and https_proxy, this is good convention / de-facto standard and there are numerous applications / libraries are honoring this network settings, what's the logic you don't think it's good idea? @tsloughter

without the http_proxy and https_proxy support, the rebar app can't work transparently to different network environment, and app layer has to take care of network config in rebar.config, that kind of coupling the app with network environment is very bad situation.