Error handling in getLatestPackageSetsTag could be improved

[hdgarrood]

There are a couple of slight oddities with the error handling in Spago.GitHub.getLatestPackageSetsTag.

Firstly, in getLatestRelease1 (the one which uses the GitHub API), if the request fails, the string "rel1" is prepended to the message which is logged as a warning (i.e. is visible without passing the -v flag). If you turn off your wifi / unplug your ethernet cable, delete (or temporarily rename) your global cache, and run spago init, you get output like this:

[...]
[warn] rel1Could not reach GitHub. Error:

HTTPError (HttpExceptionRequest Request {
  host                 = "api.github.com"
[...]

which seems unintentional. The string rel2 is included in a debug message in getLatestRelease2, which suggests that maybe the rel1 was intended to be a debug message too.

Secondly, the errors are printed unconditionally, which means that if they all fail you get five errors printed (or ten, with -v). Since it's not actually fatal if all of these attempts to fetch the latest tag file fail, perhaps we should turn all of these warning log messages into debug log messages, and also only log the last failure if they all fail? I think only logging the last failure if they all fail may have been the intention at one point anyway, judging by lines 84-86:

    liftIO (downloadTagToCache env) >>= \case
      Left (err :: SomeException) -> logDebug $ display $ Messages.failedToReachGitHub err
      Right releaseTagName -> writeTagCache releaseTagName

(note that the retrying happens inside downloadTagToCache).

[f-f]

I believe that (i.e. I did not look at the code yet) if we go for the strategy described in #657 (comment) then we can probably just get rid of all this code that gets the latest package set tag? This is because the work of getting the latest set is already being done by the package-sets CI.

However @nwolverson introduced the possibility to upgrade alternate package sets in #671 (which also introduces some of the logs you refer to), which means that Spago suddenly needs to support arbitrary repos containing sets - this means that there should be a way to support them. I think we have a couple of paths here:

1. add the requirement for other sets to have a latest-compatible-sets.json in their master branch. Supporting this should be as easy as rebasing Add latest-compatible-sets.json file package-sets#702. Even when that is missing the fallback behaviour is still going to pick a compiler-compatible set, but this still means that the package set is not being "upgraded"
2. keep this logic in Spago as a fallback after "try to fetch the latest-compatible-sets.json" but before "just pick the base set for the compiler"

My preferred option would be 1, because it's a much neater solution going forward and it's less terrible code to maintain (I'm really not proud of this part of the codebase 😄)

[hdgarrood]

Option 1 sounds good to me, although I think we might want the fallback behaviour to differ based on whether we are within spago init or spago upgrade-set: if we are in spago init then picking the default set for our compiler version should be fine, but if we are in spago upgrade-set then using that fallback could lead to the set being downgraded. Perhaps the fallback in the upgrade-set case should be taking the latest version in the repo without regard to the current compiler version?

[f-f]

Perhaps the fallback in the upgrade-set case should be taking the latest version in the repo without regard to the current compiler version?

This is the current behaviour - but this would mean keeping around this code hitting GitHub right?

[hdgarrood]

Right, but that doesn't seem too bad to me. We need to hit GitHub to get the latest-compatible-sets.json file anyway, so I think the new code probably won't differ all that much - at least, in the version of it that I'm imagining.

[f-f]

@hdgarrood there is a difference: we won't get ratelimited when hitting the raw.githubusercontent.com domain (i.e. when fetching the latest-compatible-sets.json), but we do get ratelimited when trying to hit the API or get the latest release by looking at the redirection header (i.e. the current rel1 and rel2 methods)

So the main issue I have with the current code here is not that it looks ugly, but that it doesn't work reliably.
But maybe it's also ok to keep it as the last fallback 😄