WGET doesn't support range requests, so we should ignore them when using it

[omefire]

Ignore HTTP range requests when using 'wget', as it doesn't support them and would instead fail.

[mention-bot]

@omefire, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edsko, @dcoutts and @bennofs to be potential reviewers

[ezyang]

What's your test plan for the change?

[omefire]

As mentionned in #3799, I'm not sure what user input elicits this behavior, and I'd be glad if anyone could point me to how I can exercise this code path.
I'm still looking ...

[ezyang]

What if you made a unit test that just called this directly? Seems like a bit easier to test then.

[dcoutts]

@omefire so this code path gets exercised by secure repositories during cabal update. If you use cabal head then edit your ~/.cabal/config to alter this bit:

repository hackage.haskell.org
  url: http://hackage.haskell.org/

and add secure: True, like:

repository hackage.haskell.org
  url: http://hackage.haskell.org/
  secure: True

Then do an initial update. This time it'll download the full index, so no range request. Now on subsequent updates if the index has changed then it'll just download the new tail of the index and this is where the range request comes in.

[23Skidoo]

@omefire Can you please use rebase instead of merge to get rid of spurious merge commits?

[omefire]

@23Skidoo rebased!

[23Skidoo]

This is only a partial fix. We should also print a warning when the wget transport is used with range requests

[23Skidoo]

This is only a partial fix. We should also print a warning when the wget transport is used to do range requests, pointing the user towards curl or plain-http.

Something like this:

$ cabal update
Warning: the 'wget' transport currently doesn't support range
requests, which wastes network bandwidth. To fix this, set
'http-transport' to 'curl' or 'plain-http' in '~/.cabal/config'. Note
that the 'plain-http' transport doesn't support HTTPS.

[omefire]

@23Skidoo got it. Will address it!

[omefire]

@dcoutts Thanks for explaining this, that does help a lot.

[ezyang]

@dcoutts So how, exactly, would you write a test for this ;)

[omefire]

In order for this code path to be hit, ~/.cabal/config should contain secure: False as mentioned by @dcoutts , and cabal should be run with wget selected as http-transport: cabal --http-transport=wget update --verbose.

[omefire]

I don't think we can write a unit test for this, as it requires to fake updates to the remote repo.
Maybe the route to go is with an integration test. I don't know if this user case is worth the trouble though.

[BardurArantsson]

@omefire FWIW, I agree. At some point the effort of testing just outweighs the benefits and I agree with you that that is the case here.

[ezyang]

OK, sorry, I failed to see that the ticket had updated. @omefire originally posted a suggested "fix" to get wget understanding range headers, I asked for a test, but the patch does something different now. I guess the fix didn't work. IMO the new code doesn't need a test.