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

Fix all julia 0.7 issues #227

Merged
merged 13 commits into from
Oct 18, 2018
Merged

Fix all julia 0.7 issues #227

merged 13 commits into from
Oct 18, 2018

Conversation

crbinz
Copy link
Contributor

@crbinz crbinz commented Sep 7, 2018

As far as I can tell, I caught everything (at least, tests pass and I'm getting no warnings). This is a combination of #217 and #215, along with the remaining changes.

By the way, this drops support for versions < 0.7, which seems important to mention.

carlobaldassi and others added 11 commits July 22, 2018 19:33
Tests pass on both 0.6 and 0.7; some tests however
are disabled since they are still failing.
They are no longer singletons in Julia 0.7.
Re-enable related tests.
Also improve annotations for some other issues.
triggered by JLDArchives tests
@crbinz crbinz changed the title Julia 0.7 changes Fix all julia 0.7 issues Sep 7, 2018
@carstenbauer
Copy link

Thanks so much for this! Is there anything that I could help you with?

@egoecho
Copy link

egoecho commented Sep 14, 2018

Could this package also read the .jld file saved in Julia 0.6 after the update, especially for Char data type? I mention it because of this JuliaIO/JLD2.jl#110 . I personally have fixed the compatible errors in Julia 0.7 after some simple grammar changes, however, I still faced this problem: the Char data saved in Julia 0.6 cannot be read in Julia 0.7, and vise versa. I guess the Char data type are presented differently in Julia 0.6 and Julia 0.7, as bits('A') output different results between the two Julia versions.

@crbinz
Copy link
Contributor Author

crbinz commented Sep 14, 2018

@crstnbr to be quite honest, this got me to the point where I can use the package in v0.7 with no warnings or errors, and unless the package gets more attention, I will probably just move to something else (JLD2?). I don't know enough about JLDArchives to know how to fix the travis failures, so if you wanted to tackle that, maybe this PR would then be in good enough shape to merge?

@egoecho I don't know - feel free to try by checking out this PR.

@kzapfe
Copy link

kzapfe commented Oct 9, 2018

Mmmh... as of october 9, 2018, I cannot compile JLD on Julia 0.7.0 yet. Still have all those errors about SimpleVector not being defined.

@crbinz
Copy link
Contributor Author

crbinz commented Oct 9, 2018

@kzapfe compilation failed using this PR?

@kzapfe
Copy link

kzapfe commented Oct 9, 2018

@crbinz : How should I check that? Is it not allready merged with the Current repo? My Julia 0.7.0 is under two hours fresh. I guessed that all the packages have the adecuate patches as the time being.

@crbinz
Copy link
Contributor Author

crbinz commented Oct 9, 2018

@kzapfe this is an open PR, which means it has not been merged. You can find instructions for using this PR locally here.

@kzapfe
Copy link

kzapfe commented Oct 9, 2018

Mmm. The instructions there say that I have to be able to locate somewhere around a "command line instructuions" button, after "clicking on the pull request". I did that and I am back into this conversation and I cannot find the mentioned button here or in commit/checks/files changed....

@crbinz
Copy link
Contributor Author

crbinz commented Oct 9, 2018

You can just do

git fetch origin pull/227/head:pr-277
git checkout pr-277

in ~/.julia/dev/JLD/ in order to check out this PR. This assumes you have already done

]dev JLD

from the Julia REPL.

@kzapfe
Copy link

kzapfe commented Oct 9, 2018

Thanks, @crbinz ... Now I have to find where my equivalent of .julia/dev/JLD is, because I put the Linux binaries for v0.7 in my home directory but I do not have .julia/dev/jld.. I do have a v0.3, v0.4 and v0.6 though...

@kzapfe
Copy link

kzapfe commented Oct 9, 2018

Okey, it still is not working. Just to clarify: ]add JLD
is not the adecuate command to obtain the full github clone. What must be typed is

]develop JLD

and then I can do the git fetch thing, otherwise git will complain that it doesn't find its needed directories and files to work with.
Okey, now I am sure that I am working with this PR, as I got the directory as @crbinz stated, a
.julia/dev/JLD one. When In Julia 0.7.0 I do
] test JLD
I know that I am working on that repo, asi it states

 Warning: Deprecated syntax `try without catch or finally` at /home/karel/.julia/dev/JLD/src/JLD.jl:999.

but still I have the same errors as before:

ERROR: LoadError: LoadError: UndefVarError: SimpleVector not defined
Stacktrace:
...

@crbinz
Copy link
Contributor Author

crbinz commented Oct 10, 2018

I think you have not actually checked out this PR, since the warning and error are a) the same as before, and b) specifically fixed in this PR. Did you do both git commands above?

@kzapfe
Copy link

kzapfe commented Oct 10, 2018

Oh, yeah... I forgot the checkout line...

Now it loads without problem it seems, and loads a test file. Thanks for the patiente @crbinz .
This patch MUST go into JLD for Julia 1.0.0 OR they must accept some backwards compatibilty...

@montyvesselinov
Copy link

When this will pulled? The patch works.

@JeffBezanson
Copy link
Contributor

I'll try to get CI passing.

@JeffBezanson
Copy link
Contributor

JeffBezanson commented Oct 18, 2018

I believe we need to update the format version number due to changes in Expr and Char (JuliaIO/JLDArchives.jl#17 (comment)). It looks like that has not been done since 2014! I guess instead we could try to retain compatibility by converting Chars to the old representation when writing, but then they won't be mmap-able.

@JeffBezanson
Copy link
Contributor

Ok, I think the best thing to do is merge this now then make incremental improvements after.

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.

8 participants