Use rebar_packages_cdn for fetching package resource #2479

belltoy · 2021-01-30T16:44:07Z

Not only r3_hex_repo:get_tarball need to use rebar_packages_cdn, but also all the other exported functions from r3_hex_repo.

Since only get_package and get_tarball are used, I apply cdn option with them but the rests.

Maybe this should be a config option inside the vender hex_core.

Not only `r3_hex_repo:get_tarball` need to use `rebar_packages_cdn`, but also all the other exported functions from `r3_hex_repo`. Since only `get_package` and `get_tarball` are used, I apply cdn option with them but the rests. Maybe this should be a config option inside the vender hex_core.

tsloughter · 2021-01-30T16:50:54Z

Hm, I had to look into the code because I don't remember much of how this is supposed to work, particularly in relation to the support for a priority list of repos that exists now.

But first thing I noticed is in rebar3.erl we do:

HexCDN = case os:getenv("HEX_CDN") of
                 false -> ?DEFAULT_CDN;
                 CDN -> CDN
             end,
    State2 = rebar_state:set(State1, rebar_packages_cdn, HexCDN),

So I'm not sure this is needed because it should be setting rebar_packages_cdn to the right value on startup. Are you getting different results when trying to use it?

tsloughter · 2021-01-30T16:51:36Z

Oh wait, this would mean you couldn't set rebar_packages_cdn but had to use the OS var HEX_CDN...

belltoy · 2021-01-30T16:59:00Z

Yes, the state was set on startup, but not use it correctly when performing a request to repo_url. The only one place to use it was performing get_tarball request. It should be used for all requests to the repo_url. That's the option mean to be, right?

In my patch, I get the rebar_packages_cdn from the state and replace repo_url to it before performing request get_package.

ferd · 2021-01-30T21:27:38Z

I think this option is probably a leftover of the time before we started supporting many indexes and mirrors as part of config and doesn't really make sense in a world where you can have 3-4 hex configs at once but would override it with a single CDN URL. See http://rebar3.org/docs/configuration/configuration/#hex-repos-and-indexes

I'm not sure we can keep supporting the HEX_CDN behaviour correctly with that stuff.

belltoy · 2021-01-31T03:59:25Z

According to #2204 and #1884, and the hex.pm docs, the name of rebar_packages_cdn should means the mirror of the official hex.pm repo.
Most people use the default repo, so a simple HEX_CDN environment variable would works fine for it. For compatibility and convenience, I think the rebar_packages_cdn being applied to the default hex repo only should be the right thing to do.

~~If this make sense, I will update the codes in this PR.~~ updated 2ea46f2.

This patch should fix #2346 as well.

gilbertwong96 · 2021-02-01T06:32:42Z

I think this option is probably a leftover of the time before we started supporting many indexes and mirrors as part of config and doesn't really make sense in a world where you can have 3-4 hex configs at once but would override it with a single CDN URL. See http://rebar3.org/docs/configuration/configuration/#hex-repos-and-indexes

I'm not sure we can keep supporting the HEX_CDN behaviour correctly with that stuff.

HEX_CDN(mirror_url) is definitely useful in the china mainland where GFW exists.

belltoy · 2021-02-01T10:12:12Z

HEX_CDN(mirror_url) is definitely useful in the china mainland where GFW exists.

+1

That influence us all, who were behind that wall, deeply in daily development and production deployment.

Please help to confirm this patch soon. cc @ferd @tsloughter

ferd

The fix is fine as long as HEX_CDN does only override the default repo_url.

There are however misleading debug comments and details about the implementation that I think need to be changed to be clearer. This is a bit unfortunate because you're the one currently fixing technical debt we introduced back then. Sorry, and thanks for your work there!

If you'd rather not do more work on this PR, I'm fine doing the changes myself before merging in the main branch.

ferd · 2021-02-01T13:19:49Z

src/rebar_packages.erl

@@ -243,6 +243,7 @@ parse_checksum(Checksum) ->

 update_package(Name, RepoConfig=#{name := Repo}, State) ->
    ?MODULE:verify_table(State),
+    ?DEBUG("Getting package ~ts resource from repo ~ts", [Name, maps:get(repo_url, RepoConfig, undefined)]),


I don't know that it makes the most sense to add this debug statement here. This call is internal and can get a lot of calls, with some of this information not actually being accurate. The update_package call here for example gets a debug message when it fails in rebar_prv_upgrade which refers to the repo's name and not the URL. That's because internally, the call gets the package definition and not the actual package, and the URL is not just the base value configured here, but has a subpath that may be organisation specific as well.

This is likely to be more misleading than it helps if something goes wrong. It might make more sense to have something like the following, maybe:

Suggested change

?DEBUG("Getting package ~ts resource from repo ~ts", [Name, maps:get(repo_url, RepoConfig, undefined)]),

?DEBUG("Getting definition for package ~ts from repo ~ts", [Name, maps:get(name, RepoConfig)]),

The same as below, RepoConfig#{repo_url := Value} here is use for build the URL for making request in r3_hex_repo as a prefix. The question is its name, repo_url is just a prefix, not a hole url for a request. But it's still useful in the debug output for people who want to check the http requests detail. Maybe describe it repo_url directly won't make misleadings. So how about

Suggested change

?DEBUG("Getting package ~ts resource from repo ~ts", [Name, maps:get(repo_url, RepoConfig, undefined)]),

?DEBUG("Getting definition for package ~ts from repo ~ts via repo_url ~ts",

[Name, maps:get(name, RepoConfig, undefined), maps:get(repo_url, RepoConfig, undefined)]),

I think that can work. One of the things I might do while merging is to follow the precedent I started setting in #2375 and #2393 to make it more obvious which settings are active at download time. I'll print the configured map with private fields elided and use some caching to make sure they're not printed more than once per run or something. I don't yet know which shape I want it to take, so I'll avoid blocking your PR on this.

Thanks for the review. Debugging format won't affect most uses I guess. But they are still worth to be considered and well-designed.

src/rebar_hex_repos.erl

ferd · 2021-02-01T13:24:00Z

src/rebar_pkg_resource.erl

-    CDN = maybe_default_cdn(State),
-    case request(RepoConfig#{repo_url => CDN}, Name, Vsn, ETag) of
+    RepoUrl = maps:get(repo_url, RepoConfig, undefined),
+    ?DEBUG("Downloading package ~ts from repo ~ts", [Name, RepoUrl]),


Same comment review here about the debug message. This isn't accurate and can be misleading. I understand you want to see the hex cdn being active, but this isn't a good way to get it because for other issues (i.e. people configuring the hex repo config without that URL), it actually obscures the information that's going on.

Do note that the package may also not be downloaded, but just fetched from cache. The debug call about downloading should happen nearer to the actual download call.

I agreed with you about this. repo_url isn't the actual requesting URL but only a prefix endpoint. I think we can debug repo_url right before calling r3_hex_repo:some_function which do the actual request.

But here is the appropriate place to print debug info I think, because the next request call do the actual http request to the tarball via r3_hex_repo:get_tarball. And more, the next debug info right after the request call will show the actual place the tarball got from, which was added by me.

{ok, Body, NewETag} -> ?DEBUG("Downloaded package from repo ~ts, caching at ~ts", [CDN, CachePath]),

I notice that the RepoConfig#{repo_url := URI} was used by the build_url in vendored/r3_hex_repo.erl, which means repo_url here must have a value.

The hard thing to me is how to describe this debug info, does it acceptable below?

Suggested change

?DEBUG("Downloading package ~ts from repo ~ts", [Name, RepoUrl]),

?DEBUG("Making request to get package ~ts tarball from repo ~ts via repo_url ~ts",

[Name, maps:get(name, RepoConfig, undefined), maps:get(repo_url, RepoConfig, undefined)]),

src/rebar_state.erl

belltoy · 2021-02-01T14:09:25Z

Thanks for the review, I will update the PR soon.

…epo_url

belltoy · 2021-02-02T05:39:29Z

That's why I need to apply HEX_CDN on r3_hex_repo:get_package()

ferd

I'm approving this. I'll try and run it tonight (I've got a few other changesets to fold in) and see if I end up doing a few changes as mentioned in the review, such as tweaking the debug output to line up with newer approaches, and possibly moving the new rebar_state function to utils but as far as what we expect of a contributor to be done, this is good to go.

Thanks for the contribution :)

ferd · 2021-02-05T00:57:07Z

The changes for formatting will be a bit more significant (I caught a weird edge case where we show private keys in debug output) so I'll make a follow-up PR to cover this one. Merging as is.

ferd · 2021-02-05T01:16:10Z

follow-up in #2484

belltoy added 2 commits January 31, 2021 00:40

Print more details when getting packages data from repo url for debug

038a9f8

belltoy force-pushed the master branch from 7ad49c1 to 2ea46f2 Compare January 31, 2021 05:14

ferd requested changes Feb 1, 2021

View reviewed changes

Apply HEX_CDN (mirror url) to the default public hex repo only

46de64d

belltoy force-pushed the master branch from 2ea46f2 to 46de64d Compare February 1, 2021 15:09

More appropriate debug info when making requets to a url built from r…

aa0660d

…epo_url

belltoy force-pushed the master branch from 8bf4b8d to aa0660d Compare February 1, 2021 17:10

ferd approved these changes Feb 2, 2021

View reviewed changes

ferd merged commit 5603911 into erlang:master Feb 5, 2021

ferd mentioned this pull request Feb 5, 2021

Sanitize repo configuration in DEBUG output #2484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rebar_packages_cdn for fetching package resource #2479

Use rebar_packages_cdn for fetching package resource #2479

belltoy commented Jan 30, 2021

tsloughter commented Jan 30, 2021

tsloughter commented Jan 30, 2021

belltoy commented Jan 30, 2021 •

edited

Loading

ferd commented Jan 30, 2021

belltoy commented Jan 31, 2021 •

edited

Loading

gilbertwong96 commented Feb 1, 2021

belltoy commented Feb 1, 2021

ferd left a comment

ferd Feb 1, 2021 •

edited

Loading

belltoy Feb 1, 2021 •

edited

Loading

ferd Feb 1, 2021

belltoy Feb 1, 2021

ferd Feb 1, 2021

belltoy Feb 1, 2021 •

edited

Loading

belltoy commented Feb 1, 2021

belltoy commented Feb 2, 2021

ferd left a comment

ferd commented Feb 5, 2021

ferd commented Feb 5, 2021

	?DEBUG("Getting package ~ts resource from repo ~ts", [Name, maps:get(repo_url, RepoConfig, undefined)]),
	?DEBUG("Getting definition for package ~ts from repo ~ts", [Name, maps:get(name, RepoConfig)]),

	?DEBUG("Getting package ~ts resource from repo ~ts", [Name, maps:get(repo_url, RepoConfig, undefined)]),
	?DEBUG("Getting definition for package ~ts from repo ~ts via repo_url ~ts",
	[Name, maps:get(name, RepoConfig, undefined), maps:get(repo_url, RepoConfig, undefined)]),

	?DEBUG("Downloading package ~ts from repo ~ts", [Name, RepoUrl]),
	?DEBUG("Making request to get package ~ts tarball from repo ~ts via repo_url ~ts",
	[Name, maps:get(name, RepoConfig, undefined), maps:get(repo_url, RepoConfig, undefined)]),

Use rebar_packages_cdn for fetching package resource #2479

Use rebar_packages_cdn for fetching package resource #2479

Conversation

belltoy commented Jan 30, 2021

tsloughter commented Jan 30, 2021

tsloughter commented Jan 30, 2021

belltoy commented Jan 30, 2021 • edited Loading

ferd commented Jan 30, 2021

belltoy commented Jan 31, 2021 • edited Loading

gilbertwong96 commented Feb 1, 2021

belltoy commented Feb 1, 2021

ferd left a comment

Choose a reason for hiding this comment

ferd Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

belltoy Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

ferd Feb 1, 2021

Choose a reason for hiding this comment

belltoy Feb 1, 2021

Choose a reason for hiding this comment

ferd Feb 1, 2021

Choose a reason for hiding this comment

belltoy Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

belltoy commented Feb 1, 2021

belltoy commented Feb 2, 2021

ferd left a comment

Choose a reason for hiding this comment

ferd commented Feb 5, 2021

ferd commented Feb 5, 2021

belltoy commented Jan 30, 2021 •

edited

Loading

belltoy commented Jan 31, 2021 •

edited

Loading

ferd Feb 1, 2021 •

edited

Loading

belltoy Feb 1, 2021 •

edited

Loading

belltoy Feb 1, 2021 •

edited

Loading