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

When Compiling serialize seperately per julia version #315

Merged
merged 5 commits into from
Dec 9, 2020

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Dec 4, 2020

Fixes #288 (and its duplicate #309).

I have tested locally that this works to fix that problem.
It's hard to test it in CI since it requires starting multiple julia instances.

Project.toml Outdated Show resolved Hide resolved
@oxinabox
Copy link
Contributor Author

oxinabox commented Dec 4, 2020

oops looks like i fixed it on an old version, rebasing and retest now

@oxinabox
Copy link
Contributor Author

oxinabox commented Dec 4, 2020

confirm still fixed

Project.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@fchorney fchorney left a comment

Choose a reason for hiding this comment

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

LGTM

@iamed2
Copy link
Member

iamed2 commented Dec 4, 2020

Can we make this serialize based on major.minor? Or can serialization change in patch versions?

@oxinabox
Copy link
Contributor Author

oxinabox commented Dec 4, 2020

It probably can change based on patch versions if there is bug in how serialisation is done that needs changed to the format to fix, which is then backported.
(I know I have seen some bugs which resulted in such changes idk if backported though)
More importantly if you are using Julia nightly/source build it can definitely change based on the pre-release build suffix.

I feel like keeping things simple and just using the whole VERSION is better than trying to be clever.
It makes it marginally slower to install and uses a tiny bit more harddrive.

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.

Eventually we'll probably change the approach here to use scratch spaces: #295 (comment)

src/tzdata/TZData.jl Outdated Show resolved Hide resolved
@oxinabox oxinabox requested a review from omus December 9, 2020 20:05
@omus omus mentioned this pull request Dec 9, 2020
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.

Looks good. Just going to get CI working to ensure everything is working as expected here

@codecov-io
Copy link

codecov-io commented Dec 9, 2020

Codecov Report

Merging #315 (fb852db) into master (a314759) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #315   +/-   ##
=======================================
  Coverage   93.73%   93.73%           
=======================================
  Files          32       32           
  Lines        1532     1532           
=======================================
  Hits         1436     1436           
  Misses         96       96           
Impacted Files Coverage Δ
src/tzdata/build.jl 63.63% <100.00%> (ø)

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 a314759...fb852db. Read the comment docs.

@omus
Copy link
Member

omus commented Dec 9, 2020

I'm quite sure the benchmark failure is due to the compiled files being in different directories. As a build isn't triggered when running against the old commit then the compiled files are missing according to the base version. As I'm not expecting performance issues from these changes I'll go ahead and merge this.

@omus omus merged commit 53d02f9 into JuliaTime:master Dec 9, 2020
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
5 participants