Turn of inlining when computing coverage (WIP)

[fingolfin]

It can also be explicitly turned on or off via the new keyword argument
inline for 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.

[fingolfin]

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 inline for the option is good. Perhaps it should be can_inline to match Base.JLOptions().can_inline (but then shouldn't coverage be code_coverage as well). Anyway, I don't much care either way, and will use whatever name you folks prefer :)

[tkoolen]

Base this on Base.JLOptions().can_inline, similar to other options?

[fingolfin]

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

[fredrikekre]

It feels like we shouldn't have to fix this Julia bug (?) here in Pkg.

[KristofferC]

Did coverage work ok in 0.6 with inlining off?

[fingolfin]

@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...

[KristofferC]

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 Base.JLOptions().can_inline option to the test worker. It would be good to add a blurb to the documentation about this.

[fingolfin]

@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 :-).

[KristofferC]

My point is that coverage is on by default in the CI scripts.

[fingolfin]

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)

[ararslan]

I think, for now, the best thing is to just propagate the Base.JLOptions().can_inline option to the test worker.

See JuliaLang/julia#29858

[fingolfin]

While I agree that changing this in Julia might be better, I wonder if adding this in here merging PR #870 wouldn't get this change much quicker into the hands of users? At least in my imagination, I'd expect Pkg.jl it to be easier to make a new bugfix release of Pkg.jl than of Julia itself. Then again, that might be an incorrect assessment?

[fingolfin]

Err, and with "this", I actually meant PR #870, not this PR here. Apologies for the confusion.