Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to using a HTTP source #23

Merged
merged 1 commit into from
Mar 19, 2016
Merged

Switch to using a HTTP source #23

merged 1 commit into from
Mar 19, 2016

Conversation

omus
Copy link
Member

@omus omus commented Mar 17, 2016

Previously we were downloading individual uncompressed tzdata files directly from IANA's FTP server. Unfortunately, downloading from this FTP server periodically results in:

curl: (56) Recv failure: Connection reset by peer

We managed to work around these connection issues by retrying downloads but the FTP server was still problematic for machines behind restricted networks (Note: passive FTP can use a wide range of ports).

This PR switches the build process to download tzdata from an official IANA HTTP server which appears to be much more reliable and won't cause issues with restrictive networks. In order to support this change the we needed to be able to decompress the tar.gz which requires a small additional step in the build process.

Closes #11

run(`tar xvf $tzdata -C $TZDATA_DIR $REGIONS`)
rm(tzdata)
@unix_only function extract(archive, directory, files)
run(`tar xvf $archive --directory=$directory $files`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be -xvf? Also, maybe remove the v.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty normal for tar not to have the hypen:

The first argument to tar should be a function; either one of the letters Acdrtux, or one of the long function names. A function letter need not be prefixed with ``-'', and may be combined with other single-letter options

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the verbose option I think it's rather useful here as it shows which individual tzdata files we're extracting:

INFO: Extracting TZ data
x africa
x antarctica
x asia
x australasia
x europe
x northamerica
x southamerica

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about tar

@iamed2
Copy link
Member

iamed2 commented Mar 17, 2016

Should $directory and $files be quoted in case they contain spaces?

@iamed2
Copy link
Member

iamed2 commented Mar 17, 2016

Has this been tested with GNU and BSD tar? I can't find the --directory option in the BSD tar man page.

@iamed2
Copy link
Member

iamed2 commented Mar 17, 2016

We should try tar in addition to 7zip on Windows in case they have tar installed.

end

extract(archive, TZDATA_DIR, REGIONS)
rm(archive)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessarily aggressive to remove the archive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to remove the archive. The file is being downloaded to a temporary file so this is just cleanup. You think it would be useful for debugging to just leave the archive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Plus a weird combination situation I came up with in my head.

@omus
Copy link
Member Author

omus commented Mar 17, 2016

@iamed2 quoting happens automatically with Cmd objects. You can always see how this works by displaying the Cmd object.

I haven't personally tested in with BSD tar but I believe --directory is safe to use as that is what is BinDeps.jl uses for unpack_cmd.

@iamed2
Copy link
Member

iamed2 commented Mar 17, 2016

--directory seems to work in practice for me. Weird that it isn't in the man page.

@omus
Copy link
Member Author

omus commented Mar 17, 2016

I'll note that GNU tar does mention it in the man pages. It just seems to be BSD that doesn't mention it. If you think it would be clearer -C is mentioned in the man page for BSD and GNU.

@iamed2
Copy link
Member

iamed2 commented Mar 17, 2016

--directory is clearer. If for some reason someone runs on a system where that fails, they can file an issue and we'll change it.

Previously we were downloading individual uncompressed tzdata files
directly from IANA's FTP server. Unfortunately, downloading from this
FTP server periodically results in:

    curl: (56) Recv failure: Connection reset by peer

We managed to work around these connection issues by retrying downloads
but the FTP server was still problematic for machines behind restrictive
firewalls (Note: passive FTP can use a wide range of ports).

In order to address both of these problems the build script was reworked
to attempt to download the compressed tzdata from the HTTP server first.

Note that the `extract` function is based off of BinDeps.jl's
`unpack_cmd`. I was going to use BinDeps directly but we need the
additional functionality of only extracting a subset of files.
@omus omus self-assigned this Mar 19, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.647% when pulling ba5724c on http-source into 61d8299 on master.

@omus
Copy link
Member Author

omus commented Mar 19, 2016

The Julia package evaluator found that the test cases for TimeZones.jl failed for v0.4 starting 2016-03-17. The failure was due to the "Connection reset by peer" error we get from the FTP source.

omus added a commit that referenced this pull request Mar 19, 2016
Switch to using a HTTP source
@omus omus merged commit 9b3db2c into master Mar 19, 2016
@omus omus deleted the http-source branch March 19, 2016 23:28
@quinnj
Copy link
Collaborator

quinnj commented Apr 19, 2016

Catching up on old emails; +1 to this!

omus pushed a commit to invenia/TimeZones.jl that referenced this pull request Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants