New option hexpm-mirrors, to allow specifying several mirrors and change default builds.hex.pm #156
Conversation
|
I'm Ok-ish with this change, though I feel that the action shouldn't care about cache. Specifically, you're doing this for Hex (Elixir), but e.g. not rebar3 (Erlang), and if we go down that road we might end up with something outside of our original scope. I believe we've discussed this with other maintainers, in the past (@starbelly, @ericmj, @wojtekmach) and ended up not including it. I'll search the issues and pull requests. |
|
From #45 I can find https://gist.github.com/eproxus/d74315864fd6897bb47741e8de5bc980; and we ended up not including cache-related instructions in the README. |
|
I'm also thinking that somebody comes along and wants to add cache for |
|
The main issue here is that These failures represent most if not all recent failures on the master branch for AtomVM project and a lot of failures on PRs OTOH, setup-* actions definitely should handle cache, the official ones are automated cache providers for packages. So maybe here the name cache-hexpm is misleading as it's about caching downloads, not built items (although arguably, it caches built mix archives for rebar & hex). |
Are these CDN -related? What types of issues are we talking about? (because the assets we use are behind a CDN, already)
I meant " ... OTOH if what you're providing is caching of the action elements itself, I'm more receptive. I didn't look at the details of the implementation (it's late; this wasn't discussed before, ...) but maybe I was mislead exactly because of the option name. In that case, why would we need an option for this? If cache is working and makes building faster and more consistent, shouldn't it simply be integrated into the action? |
If it does, could we separate these from the previous ones? The action's elements themselves (?) |
a64088a to
b908607
Compare
|
I double-checked and actually, this caches the unzipped hex.ez. hex.pm is supposedly a CDN, but there have been a lot of issues recently. |
|
Great, thanks. I'll wait to hear from others until we decide how to move forward (and then do a proper review). |
b908607 to
2791107
Compare
|
Drawing from this discussion, let's consider an alternative implementation. |
|
My question still holds: "... why would we need an option for this? If cache is working [from your PR onward] and makes building faster and more consistent, shouldn't it simply be integrated into the action?" meaning... why does your new behaviour simply not become the default? What are we expected to solve with an option? |
This is strange to me because repo.hex.pm and github assets use the same CDN and will hit the same POPs/data centers. Are you using github runners or your own? I haven't heard any other reports of github runners having network issues to repo.hex.pm. I also cannot see any substantial amount of errors in our metrics. Are you having more success using mirrors? |
Yet it does happen when the action matrix is large enough.
This is GitHub runners. I even had the problem running the CI to test my changes. See for example: I didn't have any problem with a mirror, but I didn't test it sufficiently. My plan was to use default CDN with GH cache. |
|
@pguyot, have you tested the result of this change, with your (currently failing) workflow, to make sure it solves the issues you want it to? (you can replace Can you maybe post the results here, so we can check the difference? Thanks. |
5812838 to
7214a8a
Compare
|
FYI, errors also occur with https://cdn.jsdelivr.net/hex without cache. |
I am confused. The PR code now makes caching of |
You're right; my text was probably not super explicit, but I did ask:
i.e. why would https://github.com/erlef/setup-beam/pull/156/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R36 be required as an option. Do you foresee a case where not using the cache, as you propose, would be better? |
This uses some space in the 10GB that GitHub provides for caching. About 100 MB. That's the reason I considered parsing ";cache" at the end of the URL. But this could be removed and the logic simplified if you think it's better, the complexity might indeed not be worth it. |
|
Yeah, I believe if we move forward (still waiting on @starbelly and @wojtekmach to say something - hopefully they will, if they have time; don't know if @ericmj has anything else to say) we should remove the option and simplify the change as much as possible. 100 MB, out of the 10 GB, is acceptable, I guess. |
|
I don't recall previous conversations around this but in my opinion built-in support for caching seems like a good idea, I'd expect most people would like to use it most of the time. I believe I'm caching deps on most if not all of my projects. I'd even say cache "on" by default is a good idea but we need to consider drawbacks. Are there any?
This is a very fair concern. I'd say the action should be concerned with caching only the things it downloads by itself. So I think we'd cache rebar but not rebar PLTs, one would need to do that themselves, which is not complicated. I'm not sure about coupling caching with mirroring though, I think these are orthogonal concerns. For example, if we'd ever want to cache the |
|
I think we all agree that the action could/should cache the elements it depends on, but stuff like PLTs, consumer-specific dependencies, etc. should not be included (which is what I thought this pull request was for, initially). Is it possible to separate these, @pguyot? Or is it a consequence of your implementation that the Hex cache folder is consumed wholly. |
7214a8a to
443b20a
Compare
It does not and never did cache stuff like PLTs and consumer-specific dependencies. |
|
@pguyot, I think this is getting close to ready. We're just missing some small things:
|
Caching was only meant to cope with poor performance of `repo.hex.pm`. Users can simply use the mirrors feature and put `cdn.jsdelivr.net/hex` as a fallback to get reliable builds. Also change default mirrors list to only include `repo.hex.pm` as requested by @ericmj, so default behavior is unaffected by this PR. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Sorry, this PR is not about retries. We shouldn't retry mirrors.
|
We will rebase+force-push once everything's dealt with. I added it to my list of TO-DO because it's something that's missing. FWIW, this pull request is changing context and scope so often is difficult to keep track of what's requested. I'd advise to have a discussion with the maintainers if you intend to change something else (that's not requested by us) in order to ease the review process. Hopefully, and if nothing further changes I will have a review result ready soon. |
|
I'd prefer repo.hex.pm to not only be the default, but the fallback in any case - except if it's already part of the chosen mirrors, especially because it's the official place we get the builds from, at the moment. This way we don't need to fail for an empty mirror list, and by renaming the option as Edit: if the consumer passes no mirror, but has the option present, add the hex.pm one; if the consumer passes a list of mirrors that does not contain hex.pm, add it to the tail of the mirror list. In both cases, log an info. for what is being done. |
We'll do the rebase, resolve conflict, and force push at the end. |
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
|
@pguyot, do you intend to continue working on this? |
I can rebase and fix conflicts to clarify the intent of this PR. It really is about mirroring (pun intended) the behavior of mix's HEX_MIRROR's environment variable to specify a mirror to use instead of default repo.hex.pm. |
|
Closing as stale. Feel free to re-open when you want to get back to it. |
New option
hexpm-mirrorsto specify the mirror(s) to use. This is particularly useful in casesbuilds.hex.pmis not reliable as it proved to be recently (see #194 for example)By default, the list of mirrors is limited to
builds.hex.pm, which means this PR does not introduce any change to existing users.The PR comes with a test of the new option. However, some tests fail, but it looks like CI is broken (several tests are flappy because of recent issues with builds.hex.pm, but test "OTP 20, Elixir v1.4, Gleam , rebar3 3.6.0" looks broken).
Signed-off-by: Paul Guyot pguyot@kallisys.net