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 nightly CI tests #810

Merged
merged 4 commits into from
Sep 9, 2020
Merged

Fix nightly CI tests #810

merged 4 commits into from
Sep 9, 2020

Conversation

Liozou
Copy link
Contributor

@Liozou Liozou commented Jul 10, 2020

This (hopefully) turns the angry red cross into a nice green tick.

There are two separate commits, with two distinct goals:

  • the first commit is a genuine test fix for julia versions >= v1.6, which reflects RFC: print all types using equivalent exported aliases JuliaLang/julia#36107
  • the second commit is not a fix, it just marks as @test_broken what has been broken for a few months now on julia versions >= v1.5. I tried to correctly fix this issue but @code_typed mul!(c,A,b) yields the same thing on v1.4 and v1.5 while @code_llvm mul!(c,A,b) is different, so I have no idea how to fix it, sorry.

(The second commit is in direct conflict with 6e0936f so #783 will have to remove that commit if this PR gets merged.)

EDIT: added a third commit, see #810 (comment)

@fredrikekre
Copy link
Member

Probably need to adjust for JuliaLang/julia#37085 too, let's rerun the tests.

@fredrikekre fredrikekre reopened this Aug 31, 2020
@mateuszbaran
Copy link
Collaborator

Apparently the tests are still passing. I don't understand it now, that PR to Julia was supposed to add spaces after commas but it doesn't everywhere?

@mateuszbaran
Copy link
Collaborator

That's probably because show_typealias doesn't add spaces after commas.

@mateuszbaran
Copy link
Collaborator

JuliaLang/julia#37302 broke a test again (but all that is needed to fix it is adding one space). Do we want to fix this in this PR or a separate one?

@Liozou Liozou force-pushed the testfixshowalias branch 2 times, most recently from bf1740b to 333d666 Compare September 2, 2020 13:59
@Liozou
Copy link
Contributor Author

Liozou commented Sep 2, 2020

I fixed the whitespace issue from JuliaLang/julia#37302 and while I was at it I also marked as test_broken the ambiguity test for julia >= v1.6 because of the very recent JuliaLang/julia#36962.
Tested against master, detect_ambiguities(StaticArrays) now yields 45 method ambiguities, while detect_ambiguities(LinearAlgebra, StaticArrays) yields 114 method ambiguities and takes almost 10 minutes to run on my computer (with a decent CPU). But that's a separate issue from that which is fixed by this PR.

@mateuszbaran
Copy link
Collaborator

Thanks, I think we can merge this. 114 ambiguities is quite a lot, I'll check what they are.

@Liozou
Copy link
Contributor Author

Liozou commented Sep 2, 2020

Some of them are internal LinearAlgebra ambiguities, for instance the first I get is

(ldiv!(transA::Transpose{var"#s851", var"#s850"} where var"#s850"<:(LU{var"#s849", var"#s848"} where var"#s848"<:(StridedMatrix{T} where T) where var"#s849") where var"#s851", B::StridedVecOrMat{T} where T) in LinearAlgebra at julia/stdlib/v1.6/LinearAlgebra/src/lu.jl:402, ldiv!(transA::Transpose{var"#s820", var"#s819"} where var"#s819"<:(UpperTriangular{T, var"#s818"} where var"#s818"<:(StridedMatrix{T} where T)) where var"#s820", B::StridedVecOrMat{T}) where T<:Union{Float32, Float64, ComplexF32, ComplexF64} in LinearAlgebra at julia/stdlib/v1.6/LinearAlgebra/src/triangular.jl:769)

which has nothing to do with StaticArrays. But some of them might be genuine, I'm afraid I don't have the time to look into this precisely at the moment.

@Liozou
Copy link
Contributor Author

Liozou commented Sep 2, 2020

CI is green at least!

@c42f
Copy link
Member

c42f commented Sep 3, 2020

yields 114 method ambiguities and takes almost 10 minutes to run on my computer (with a decent CPU).
...
which has nothing to do with StaticArrays

Oh dear, the large runtime is a disappointing regression in usability for detect_ambiguities :-(

I guess we could follow the style of the test in https://github.com/JuliaLang/julia/pull/36962/files#diff-cd2de97b330251a95f7a2b119e9daf48

According to JuliaLang/julia#36962 (comment) the new ambiguities in Base were due to JuliaLang/julia#36951, so in the sense that it's a bug in Base, I think marking as @test_broken is fine until 1.6 is out.

Let me push a small refinement to this PR so that we don't forget to look back at this when 1.6 comes out.

test/ambiguities.jl Outdated Show resolved Hide resolved
@Liozou
Copy link
Contributor Author

Liozou commented Sep 8, 2020

Bump. Any reason to postpone this?

@c42f c42f merged commit b5b6d7b into JuliaArrays:master Sep 9, 2020
@c42f
Copy link
Member

c42f commented Sep 9, 2020

No, I just forgot to come back and merge it!

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.

4 participants