-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Downgrade CI #49
Downgrade CI #49
Conversation
Regression? |
for i = 1:n
#= /home/runner/work/MuladdMacro.jl/MuladdMacro.jl/test/runtests.jl:104 =#
z = (muladd)(x, i, y)
#= /home/runner/work/MuladdMacro.jl/MuladdMacro.jl/test/runtests.jl:105 =#
end == for i = 1:n
#= /home/runner/work/MuladdMacro.jl/MuladdMacro.jl/test/runtests.jl:107 =#
z = (muladd)(x, i, y)
#= /home/runner/work/MuladdMacro.jl/MuladdMacro.jl/test/runtests.jl:108 =#
end Is the regression in |
julia> @test @macroexpand(@muladd f(x, y, z) = x * y + z) ==
:(f(x, y, z) = $(Base.muladd)(x, y, z))
Test Failed at REPL[29]:1
Expression: #= REPL[29]:1 =# @macroexpand(#= REPL[29]:1 =# @muladd(f(x, y, z) = begin
#= REPL[29]:1 =#
x * y + z
end)) == :(f(x, y, z) = begin
#= REPL[29]:2 =#
($(Base.muladd))(x, y, z)
end)
Evaluated: (f(x, y, z) = begin
#= REPL[29]:1 =#
(muladd)(x, y, z)
end) == (f(x, y, z) = begin
#= REPL[29]:2 =#
(muladd)(x, y, z)
end) |
It was pointed out on Slack that it may be the line numbers? We can try stipping the line numbers before the comparison. |
Good to go now. |
julia = "1.6" | ||
Aqua = "0.8.4" | ||
Test = "1.0.0" | ||
julia = "1.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisRackauckas decided to move everything to Julia 1.10 as it is likely to be the new LTS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think such unnecessary Julia compat changes are quite unfortunate for utility packages such as MuladdMacro that are useful not only for SciML packages but the whole ecosystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's somewhat easiest to just have everything on what will be the new LTS though. At least as we're rolling out the new Downgrade CI which is a surprisingly difficult thing to get right, keeping everything simple is best. I do agree that we could in theory make exceptions for something like this repo which effectively never changes though (though its tests actually needed a change for v1.10)
No description provided.