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

Raise the min julia version to 1.3 #361

Merged
merged 11 commits into from
Oct 12, 2021
Merged

Raise the min julia version to 1.3 #361

merged 11 commits into from
Oct 12, 2021

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Oct 8, 2021

As discussed in #354 raising min version to 1.3 will let use InlineStrings.jl

@oxinabox
Copy link
Contributor Author

oxinabox commented Oct 8, 2021

1.3 tests are failing as in julia 1.3 JULIA_NUMTHREADS enviroment variable was only way to set number of threads.
But tests use julia -t instead.
I will fix on monday

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

I'd be okay with setting the minimum to 1.3.2 to clear out this:

if v"1.3.1-pre.18" <= VERSION < v"1.3.2"
using Pkg.Artifacts: do_artifact_str, find_artifacts_toml, load_artifacts_toml
# A copy of `Pkg.Artifacts.@artifact_str` where `name` is properly escaped on
# Julia 1.3.1
# https://github.com/JuliaLang/Pkg.jl/issues/1912
# https://github.com/JuliaLang/Pkg.jl/pull/1580
macro artifact_str(name)
# Load Artifacts.toml at compile time, so that we don't have to use `__source__.file`
# at runtime, which gets stale if the `.ji` file is relocated.
local artifacts_toml = find_artifacts_toml(string(__source__.file))
if artifacts_toml === nothing
error(string(
"Cannot locate '(Julia)Artifacts.toml' file when attempting to use artifact '",
name,
"' in '",
__module__,
"'",
))
end
local artifact_dict = load_artifacts_toml(artifacts_toml)
return quote
# Invalidate .ji file if Artifacts.toml file changes
Base.include_dependency($(artifacts_toml))
# Use invokelatest() to introduce a compiler barrier, preventing many backedges from being added
# and slowing down not only compile time, but also `.ji` load time. This is critical here, as
# artifact"" is used in other modules, so we don't want to be spreading backedges around everywhere.
Base.invokelatest(do_artifact_str, $(esc(name)), $(artifact_dict), $(artifacts_toml), $__module__)
end
end

src/types/timezone.jl Outdated Show resolved Hide resolved
src/tzdata/build.jl Outdated Show resolved Hide resolved
src/winzone/WindowsTimeZoneIDs.jl Outdated Show resolved Hide resolved
src/winzone/WindowsTimeZoneIDs.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Contributor Author

oxinabox commented Oct 8, 2021

I'd be okay with setting the minimum to 1.3.2 to clear out this:

Turns out there is no 1.3.2.
The last 1.3 release was 1.3.1, so that code is needed to support 1.3 at all.

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2021

Codecov Report

Merging #361 (fcf6e78) into master (22a32d4) will increase coverage by 0.97%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   93.30%   94.27%   +0.97%     
==========================================
  Files          32       32              
  Lines        1613     1607       -6     
==========================================
+ Hits         1505     1515      +10     
+ Misses        108       92      -16     
Impacted Files Coverage Δ
src/types/timezone.jl 94.28% <ø> (+2.61%) ⬆️
src/tzdata/download.jl 96.15% <ø> (-0.08%) ⬇️
src/tzdata/build.jl 68.75% <46.66%> (+5.70%) ⬆️
src/winzone/WindowsTimeZoneIDs.jl 100.00% <100.00%> (+40.90%) ⬆️
src/tzdata/version.jl 81.57% <0.00%> (-10.53%) ⬇️
src/tzdata/archive.jl 100.00% <0.00%> (ø)
src/local.jl 88.88% <0.00%> (+0.13%) ⬆️
src/build.jl 100.00% <0.00%> (+16.66%) ⬆️
... and 1 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 22a32d4...fcf6e78. Read the comment docs.

@oxinabox oxinabox requested a review from omus October 11, 2021 11:08
@omus
Copy link
Member

omus commented Oct 11, 2021

A couple more changes needed:

if VERSION >= v"1.6.0-DEV.923"
# Use Downloads.jl once TimeZones.jl drops support for Julia versions < 1.3
download(args...) = Base.invokelatest(Base.Downloads().download, args...)
else
using Base: download
end

@static if VERSION >= v"1.3"
artifact_dir = @artifact_str "unicode-cldr-$UNICODE_CLDR_VERSION"
cp(joinpath(artifact_dir, WINDOWS_ZONE_FILE), xml_file, force=true)
else
download(WINDOWS_ZONE_URL, xml_file)
end

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

Overall looks good. I've identified a few more things we can clear out due to dropping Julia 1.0 (e.g. tzdata_download and friends) but I can tackle that in another PR.

Project.toml Outdated Show resolved Hide resolved
test/thread-safety.jl Outdated Show resolved Hide resolved
@omus omus added this to the 1.6.0 milestone Oct 12, 2021
oxinabox and others added 2 commits October 12, 2021 15:53
Co-authored-by: Curtis Vogt <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@omus omus merged commit 482f3b7 into JuliaTime:master Oct 12, 2021
else
download(WINDOWS_ZONE_URL, xml_file)
end
download(WINDOWS_ZONE_URL, xml_file)
Copy link
Member

Choose a reason for hiding this comment

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

I just notice this is backwards. Will address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops my bad.

kpamnany pushed a commit to RelationalAI-oss/TimeZones.jl that referenced this pull request May 5, 2023
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.

3 participants