Improve build tools #58

omus · 2017-04-04T01:36:23Z

Numerous improvements around the TimeZones.jl build process including:

Move build.jl code into the module
Allowing users to optionally specify what version of tzdata to use
Reduce requests to IANA and Unicode servers

Additional work to complete:

~~Periodically allow downloads of windowsZones.xml (currently requires force)~~ (Support retrieval of older Windows translation XML #62)
~~Support retrieval of older versions of windowsZones.xml. There may be incompatibilities between old tzdata versions using the latest version~~ (Support retrieval of older Windows translation XML #62)
~~Avoid using Ref for keeping track of latest tzdata version and timestamp~~
Docstring for TimeZones.build
Load Windows ~~translation~~ XML at module load
Additional tests

Fixes #55

codecov-io · 2017-04-04T01:36:25Z

Codecov Report

Merging #58 into master will decrease coverage by 2.33%.
The diff coverage is 61.39%.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   90.98%   88.65%   -2.34%     
==========================================
  Files          19       25       +6     
  Lines         799      943     +144     
==========================================
+ Hits          727      836     +109     
- Misses         72      107      +35

Impacted Files	Coverage Δ
src/utils.jl	`97.56% <ø> (+2.1%)`	⬆️
src/accessors.jl	`100% <ø> (ø)`	⬆️
src/types.jl	`97.64% <ø> (ø)`	⬆️
src/tzdata/timeoffset.jl	`100% <ø> (ø)`
src/interpret.jl	`100% <ø> (ø)`	⬆️
src/local.jl	`82.6% <0%> (+3.44%)`	⬆️
src/winzone/WindowsTimeZoneIDs.jl	`0% <0%> (ø)`
src/tzdata/version.jl	`100% <100%> (ø)`
src/tzdata/compile.jl	`93.77% <100%> (ø)`
src/tzdata/TZData.jl	`100% <100%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c55948...0bc92df. Read the comment docs.

iamed2

Should probably run a build test of some kind in CI. CodeCov also seems to have choked on the tzdata folder rename.

iamed2 · 2017-04-04T20:53:01Z

src/tzdata/download.jl

+# Examples
+```julia
+julia> tzdata_url("2017a")
+"http://www.iana.org/time-zones/repository/releases/tzdata2017a.tar.gz"


Example should be HTTPS. Doctest?

Good catch. I need to switch this package over to Documenter yet

iamed2 · 2017-04-04T21:01:32Z

src/tzdata/download.jl

+    if version == "latest"
+        v = latest_version(now_utc)
+        if !isnull(v)
+            return joinpath(dir, "tzdata$(unsafe_get(v)).tar.gz")


I don't think unsafe_get exists on 0.5. This code path should probably be tested.

It doesn't exist on 0.5 but it is in Compat.jl. I added an import at the top of the file to make this clear.

iamed2 · 2017-04-04T21:02:43Z

src/tzdata/download.jl

+    archive = Base.download(url, joinpath(dir, basename(url)))  # Overwrites the local file if any
+
+    # HTTP 404 Not Found can result in a empty file being created
+    if !isarchive(archive)


Maybe differentiate between an empty file and an invalid archive file? This would also trigger on a corrupt download, right?

The comment needs to be changed. If download encounters a 404 we will still generate a file which could be empty or contain the sites 404 page. Corrupt downloads will also raise an exception as the archive shouldn't pass tests.

iamed2 · 2017-04-04T21:44:46Z

src/tzdata/archive.jl

@@ -0,0 +1,71 @@
+import Compat: @static, is_windows
+
+"""


Document keyword argument

iamed2 · 2017-04-04T21:49:46Z

Also: is Windows CodeCov possible?

tkelman · 2017-04-05T11:43:10Z

deps/build.jl

-
-info("Successfully processed TimeZone data")
+# ENV variable allows us to only download a single version during CI jobs
+build(get(ENV, "JULIA_TZ_VERSION", "latest"))


shouldn't it be the other way around, pinned by default for reproducibility of what gets installed in the usual case, but set latest (maybe as a separate matrix job) on CI to detect problems?

I think people want the latest tz files always

Then make a new package release when there's a new tzdata file available. This has been the cause of repeated test failures over the last few years. Better to stick to something that's known to work correctly, and only release new versions after they've been tested.

This has been the cause of repeated test failures over the last few years

The test failures you're referring to were mainly to do with counting the number of time zone abbreviations. That problem was fixed in 2a7a307.

Better to stick to something that's known to work correctly, and only release new versions after they've been tested.

The tests are meant to ensure that the code is working correctly and not test the the tzdata. The compilation process of converting the tz source files into serialized Julia structs also ensures that the tzdata is parsed correctly. The reason for pinning the version of tzdata for testing was to make sure we were just testing for code changes and not data changes.

End-users will want accurate time zone information which would be the latest version. It is possible that the latest version of the tzdata could include something that causes the tz compilation process to break but this has never happened to date. Additionally, with this PR I've tested the tz compilation process against older versions including 1996n which demonstrates that the tz source format is stable and unlikely to change.

End-users will want accurate time zone information which would be the latest version.

Then release new versions of the package when a new tzdata release comes out. We're talking at most a couple times a month? Users also want to be able to restore past versions of working code and reproduce results, which becomes unnecessarily difficult to do if you're downloading unversioned "latest" content from the internet at install time.

@tkelman Production code that gives correct results without having to update your package dependency tree (or possibly even your Julia version)

This is essentially a dichotomy between correct and reproducible.

it's also a question of library code vs application code. a library just needs to be correct at the time it was released, if it stops being correct due to an external resource changing, the solution is use a newer library version. applications or services that don't care about reproducibility or archival can configure the way they use libraries to depend on changing sources of external data, but a library shouldn't be forcing that choice on all users to give different results from running the same source code 6 months later.

Hmm. One possible solution is have a separate package which only handles fetching TZ files. That way, people can pin/free that and this package independently.

Linux distributions do mostly have the tzdata packaged in a pretty minimal data-only package, from what I've seen

One possible solution is have a separate package which only handles fetching TZ files. That way, people can pin/free that and this package independently.

I think this is probably the ideal solution. Unfortunately, I won't be able to do this work right away so this is what I'll do for the moment. For now, I'll update this line from "latest" to "2017b" to deal with the reproducibility problem. Users who want to always use the most up to date version automatically can currently set an env variable JULIA_TZ_VERSION=latest. I'll also be keeping a closer eye on tzdata releases so I can keep the default version up to date.

omus · 2017-04-05T16:26:54Z

Also: is Windows CodeCov possible?

Possible but not currently supported. I opened an issue for this.

omus · 2017-04-05T20:58:04Z

Window code coverage is too much trouble for this PR. I almost got it working on Julia 0.4 but newer versions of Julia are problematic: JuliaLang/julia#21289

tkelman · 2017-04-06T15:15:48Z

src/winzone/WindowsTimeZoneIDs.jl

    open(translation_file, "w") do fp
-        serialize(fp, translation)
+        serialize(fp, compile(xml_file))


what's the motivation for using the Julia serializer here? that isn't really meant for long-term storage

Serialization is used here to avoid having to re-parse the XML during the package loading. I don't need to strictly need to use serialization in this case.

I do use serialization for storing the TimeZone types. Can serialization change between Julia patch releases?

It shouldn't change between patch releases, but I'm not sure we have good tests ensuring that. It definitely changes between minor releases, and during the course of nightlies.

omus · 2017-04-07T21:53:01Z

Code is finally in a state I am more or less happy with. The code coverage drop is mostly from the introduction of more Windows specific code. PR #60 should help with that.

omus added 12 commits April 3, 2017 10:31

Avoid using String for Julia 0.4

fa0ca73

Create tools for working with tzdata archives

497ece7

Move all tz source compiliation into TZData

f55df85

Deprecate Olson module

11f5716

Refactor compile constants

2922118

Switch to Vector

fc95435

Rename TZDATA_DIR to TZ_SOURCE_DIR

8e5f0e9

Rename archive directory

a082381

DEPS_DIR constant

4701aea

Only download latest once per hour

285feeb

Version without using a custom type

e13de40

Nicer interface

c6edfea

omus self-assigned this Apr 4, 2017

iamed2 requested changes Apr 4, 2017

View reviewed changes

tkelman reviewed Apr 5, 2017

View reviewed changes

omus force-pushed the tzdata branch 3 times, most recently from 1ec486f to c66fa47 Compare April 5, 2017 19:44

omus force-pushed the tzdata branch from 1c320f2 to 96dd5ab Compare April 6, 2017 13:41

tkelman reviewed Apr 6, 2017

View reviewed changes

omus force-pushed the tzdata branch from 2de4300 to 8e3e2a0 Compare April 6, 2017 16:38

omus mentioned this pull request Apr 6, 2017

Move away from serialize #61

Closed

omus force-pushed the tzdata branch 4 times, most recently from 0d6e454 to 5770279 Compare April 6, 2017 21:17

omus added 15 commits April 7, 2017 11:04

Single download on CI runners

4279055

Add tests for archive

f5c8d98

Various review changes

86458d0

Add tests for tzdata/version

aefe905

Internal build method defaults to verbose=false

4a6ba1a

Move tzdata tests into folder

ee59270

Reorder includes

f40f6a7

Add download tests

0e4c9cc

Deserialize Windows translation once

8ffd9ab

Switch to isassigned for Ref

be4cbaa

Fix include paths

18eb27b

Add docstring for TimeZones.build

961f5e7

Add test for TimeZones.build

4d54f9c

Return version from TZData.build

698f3d9

Remove serialization for Windows translation

07bdbef

omus force-pushed the tzdata branch 2 times, most recently from c5d66c0 to 73b9d7d Compare April 7, 2017 16:27

Add support for determining active tzdata version

516cb94

omus force-pushed the tzdata branch from 73b9d7d to 8039610 Compare April 7, 2017 17:40

Add test for Etc/* FAQ example

ccf6194

omus force-pushed the tzdata branch 2 times, most recently from c3dcf65 to abc0fc5 Compare April 7, 2017 18:07

omus added 3 commits April 7, 2017 14:18

Add test for FAQ example max_year

28a21e3

Remove download failure note

f9eacea

Set default tzdata version to be 2017b

0bc92df

omus force-pushed the tzdata branch from abc0fc5 to 0bc92df Compare April 7, 2017 19:19

omus merged commit d183a73 into master Apr 7, 2017

omus mentioned this pull request Apr 7, 2017

Move tzdata download into an artifact #63

Closed

omus deleted the tzdata branch April 12, 2017 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve build tools #58

Improve build tools #58

omus commented Apr 4, 2017 •

edited

Loading

codecov-io commented Apr 4, 2017 •

edited

Loading

iamed2 left a comment

iamed2 Apr 4, 2017

omus Apr 4, 2017

iamed2 Apr 4, 2017

omus Apr 5, 2017

iamed2 Apr 4, 2017

omus Apr 4, 2017

iamed2 Apr 4, 2017

omus Apr 5, 2017

iamed2 commented Apr 4, 2017

tkelman Apr 5, 2017

iamed2 Apr 5, 2017

tkelman Apr 5, 2017

omus Apr 5, 2017

tkelman Apr 5, 2017

iamed2 Apr 5, 2017

tkelman Apr 5, 2017

iamed2 Apr 5, 2017

tkelman Apr 5, 2017

omus Apr 7, 2017 •

edited

Loading

omus commented Apr 5, 2017

omus commented Apr 5, 2017 •

edited

Loading

tkelman Apr 6, 2017

omus Apr 6, 2017

tkelman Apr 6, 2017

omus commented Apr 7, 2017

Improve build tools #58

Improve build tools #58

Conversation

omus commented Apr 4, 2017 • edited Loading

codecov-io commented Apr 4, 2017 • edited Loading

Codecov Report

iamed2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamed2 commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

omus commented Apr 5, 2017

omus commented Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Apr 7, 2017

omus commented Apr 4, 2017 •

edited

Loading

codecov-io commented Apr 4, 2017 •

edited

Loading

omus Apr 7, 2017 •

edited

Loading

omus commented Apr 5, 2017 •

edited

Loading