Turn of inlining when computing coverage (WIP) #866
Conversation
|
Note that I made it so that now any coverage test by default turns off inlining, thus most packages should get the "fix" automatically. Drawback of this is that those tests also now may run a lot slower, which might cause new problems. So one may disagree whether this is a good idea or not. I can of course change the defaults if that's deemed safer, but then tons of projects may need to explicitly turn off inlining for their coverage tests. Another argument to be made is whether the name |
0a9ac18
to
1eff52e
Compare
It can also be explicitly turned on or off via the new keyword argument `inline` for Pkg.test.
1eff52e
to
2332ac6
Compare
| @@ -1333,6 +1333,7 @@ function test(ctx::Context, pkgs::Vector{PackageSpec}; coverage=false) | |||
| --color=$(Base.have_color ? "yes" : "no") | |||
| --compiled-modules=$(Bool(Base.JLOptions().use_compiled_modules) ? "yes" : "no") | |||
| --check-bounds=yes | |||
| --inline=$(inline ? "yes" : "no") | |||
There was a problem hiding this comment.
Base this on Base.JLOptions().can_inline, similar to other options?
There was a problem hiding this comment.
I thought about doing that, too. One drawback of that would be that all packages affected by the problem at hand would have to be updated to explicitly add --inline=no to their test script(s), and of course also the default Julia scripts for Travis CI and AppVeyor would need to be adapted. I.e. far more people would have to change far more code.
That said, of course I can change it to do that shrug
|
It feels like we shouldn't have to fix this Julia bug (?) here in Pkg. |
|
Did coverage work ok in 0.6 with inlining off? |
|
@fredrikekre We can't fix a Julia bug (?) in Pkg anyway, we can merely work around it. So, consider this PR as a bandaid, to help people get working coverage reports now, as a bandaid until when (if?) Julia actually gets fixed / improved in this regard. But proper coverage tracking in compiled involving optimizations and inlining is a really difficult problem, so I would not expect a quick solution (although of course I'd love to be proven wrong); and the "official" solution may very well involve the suggestion to turn off inlining... |
|
This would turn off inlining for everyone using the default CI script by default which is not really acceptable. I think, for now, the best thing is to just propagate the |
|
@KristofferC to clarify: this would turn off inlining for the part computing coverage results. I'd argue that computing coverage results with inlining on largely makes no sense (at least with the current state of it in Julia), as the result is completely bonkers. Which is after all what motivated this PR in the first place. So I'd argue to the contrary that no disabling it is not really acceptable... Anyway, if that's what the Julia pros decide is the way to go, so be it, I don't mind; after all, I know how to get things working for my projects either way :-). |
|
My point is that coverage is on by default in the CI scripts. |
|
Ahhhh, OK -- indeed! Then I guess I have to concur with your assessment (and there just is no good way out of this that doesn't require a lot of package authors to make changes) |
|
|
While I agree that changing this in Julia might be better, I wonder if |
|
Err, and with "this", I actually meant PR #870, not this PR here. Apologies for the confusion. |
870: Transfer can_inline to subprocess r=KristofferC a=fingolfin This is meant as an alternative to PR #866 for resolving issue #865 Co-authored-by: Max Horn <[email protected]> Co-authored-by: Kristoffer Carlsson <[email protected]>
It can also be explicitly turned on or off via the new keyword argument
inlinefor Pkg.test.This is meant to resolve issue #865. I marked it as WIP as clearly this new option should be documented; but before spending time on that, I wanted to see if this approach was deemed acceptable at all.