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 qualified show rewrite #230

Merged
merged 3 commits into from
Jun 25, 2016
Merged

Fix qualified show rewrite #230

merged 3 commits into from
Jun 25, 2016

Conversation

TotalVerb
Copy link
Contributor

@TotalVerb TotalVerb commented Jun 19, 2016

This addresses the missing case in #219.

Could a new version be tagged soon? I got hit by this when updating Hiccup. Hiccup tests also didn't catch it, so luckily I double checked! (I noticed the omission in the original PR, but since tests passed I thought it might have been fixed in the interim.)

While debugging this, I noticed that the Base.Timer rewrite was untested and didn't actually work. I decided to fix that along the way.

@TotalVerb
Copy link
Contributor Author

CI failures are due to unrelated issues.

@timholy
Copy link
Member

timholy commented Jun 24, 2016

Really nice, @TotalVerb. I also got bit by this in JuliaGraphics/Cairo.jl#143. Unlike you, I got bit "for real" 😦.

The test failure on julia-0.3 (linux-only, curiously) is a little bit alarming: "Nullable{Float32} has no method matching Nullable{Float32}()". Did something else change?

@TotalVerb
Copy link
Contributor Author

I cannot reproduce the problem locally.

Julia Version 0.3.12
Commit 80aa779 (2015-10-26 12:41 UTC)
Platform Info:
  System: Linux (x86_64-unknown-linux-gnu)
  CPU: Intel(R) Core(TM) i7-3632QM CPU @ 2.20GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.3

@TotalVerb
Copy link
Contributor Author

Let's restart Travis.

@TotalVerb TotalVerb closed this Jun 24, 2016
@TotalVerb TotalVerb reopened this Jun 24, 2016
@tkelman
Copy link
Contributor

tkelman commented Jun 24, 2016

I can reproduce the error on Ubuntu 14.04, julia 0.3

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Jun 24, 2016

I wish I could say this was another symlink failure. But unfortunately I am actually testing the right package. I can't reproduce.

Does Travis rebase on master? Maybe it was another change that did this. I will rebase and check.

Edit: Unfortunately this branch is up to date already. What an annoying issue.

@TotalVerb
Copy link
Contributor Author

Also, we should fix the nightly failures. I think those are in master.

@timholy
Copy link
Member

timholy commented Jun 25, 2016

@tkelman, is the 0.3 failure dependent on this PR?

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Jun 25, 2016

Nightly failures see #232. Also shows that the 0.3 failure is dependent on this PR.

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Jun 25, 2016

v0.3 failure is hard to reproduce (I have to go through Travis) but I think I've got something licked. It is however extremely strange.

Replacing

export Nullable, NullException, isnull

with

export Nullable, NullException, isnull, eltype

worked. I suspect there might be a problem with Julia 0.3 itself.

If this is memory corruption or another Julia bug, then I can't investigate further. @tkelman, could you have a look?

@TotalVerb
Copy link
Contributor Author

To add, I don't think this PR is actually the cause of the issue. I suspect that the underlying issue is something very sensitive to small, possibly unrelated, changes. Memory corruption being the underlying issue also explains why it's so platform dependent.

@TotalVerb
Copy link
Contributor Author

Travis is green now (except for v0.5 failure, which is #232). This workaround is not ideal, but I don't want what looks like a probable Julia v0.3 bug holding up this PR, especially if it is hard to fix.

@tkelman
Copy link
Contributor

tkelman commented Jun 25, 2016

80d99b4 seems good enough for me. The reproduction was from checking out this branch of @TotalVerb's fork. Thanks for looking into it and finding a fix, @timholy and I had mentioned in person how much we both appreciate the great work you've been doing.

@tkelman tkelman merged commit 6261ff5 into JuliaLang:master Jun 25, 2016
@tkelman
Copy link
Contributor

tkelman commented Jun 25, 2016

I'm actually tempted to disallow new tags of packages that support 0.3 once master of Julia has moved on to be 0.6-dev and I change PkgEval to test vs 0.4, 0.5 and 0.6. Right now it still tests against 0.3 but doesn't report it on the web site, I manually check the commit log and so far it's been pretty much only the flaky packages that ever fail on 0.3 since most other packages have moved on to not actively supporting new tags on it any more.

@tkelman
Copy link
Contributor

tkelman commented Jun 26, 2016

Oh dear, I think this managed to break JuMP's tests on 0.3? I'm confused how though. Pinning Compat back to v0.8.1 makes them pass. cc @mlubin @joehuchette

https://gist.github.com/b57fce33157184dbd3f090b41a52bbce

@timholy
Copy link
Member

timholy commented Jun 27, 2016

Plenty of evidence that this somehow triggered corruption on 0.3; I am reminded of JuliaLang/julia#13306.

If no simple solution presents itself, I vote we retrospectively put a julia-0.4 requirement on Compat, and drop 0.3 support. Probably should retrospectively bump the version to 0.9.

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Jun 27, 2016

I do wonder whether corruption was actually introduced in this PR, or if it was always there and this PR just managed to expose it on certain configurations somehow. It seems like a method table issue, but this PR doesn't add or remove any methods. I agree that dropping 0.3 support might be the best option.

@TotalVerb
Copy link
Contributor Author

For what it's worth, I can reproduce the JuMP failure.

@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2016

I was going to propose adding upper bounds on the Compat requirement to the 0.3-supporting past tags of JuMP. If this change to Compat has introduced some un-fixable issue that only manifests on Julia 0.3, that's as good a reason as any to declare package versions that support 0.3 as frozen from now on. We should raise this somewhere visible, julia-users and maybe also julia-dev and julia-news just so people are aware.

@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2016

Likely related - JuliaDiff/TaylorSeries.jl#54 and JuliaDiff/TaylorSeries.jl#53 cc @lbenet

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Jun 27, 2016

If this problem occurs on 0.4, then that's another story. I still do not know how this PR is causing the issue. Perhaps the usage of macroexpand creates bugs? I will test a version without it and see if JuMP bugs disappear.

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Jun 27, 2016

The JuMP failure is an off-by-one error introduced in #219, that was exposed by this PR. (It was rewriting show incorrectly, but JuMP only overloaded Base.show so the problem did not manifest until this PR was merged.) Phew! Should be an easy fix.

Edit: not an off-by-one error, but some undesirable behaviour.

Edit: should be fixed by #237.

dpsanders pushed a commit to dpsanders/Compat.jl that referenced this pull request Feb 1, 2017
* Fix show rewriting

* Fix Timer rewriting and add test

* Work around v0.3 issue (possible memory corruption?)
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