Can one of the admins verify this patch?

1. This requires notes.
2. You're breaking bincompat changing those public methods
3. It's interesting to note the different approach here compared to Duhemm@fc6a337, ie. logging as late as possible with what information percolates down (Martin's way), vs logging as early as possible at ever entry point (your way). It's a trade-off between information availability and making sure a log always happen. I'm interested in what other think.

1. Indeed, I waited for the PR creation, since the PR number is often included in the notes
2. Argh, I'll need to see if I can get the javaOptions in another way to allGroupTestTasks (or somewhere else).
3. On the topic of logging the javaOptions that are used, @Duhemm 's solution is without hesitation a better solution, since it avoids code duplication. However, this can't work for 2041, since the issue is that we're not forking but javaOptions are still provided and ignored.

@dwijnand You win for reviewing while I had this open and waiting, thereby misisng your comments completely. Agree with your comments.

So, should I remove the javaOptions part and leave it to @Duhemm, or should I implement his solution by logging in ForkJava/LocalJava ?

Added notes.

@pdalpra Do we log the same (or "sufficiently" similar) information and in all the same cases by moving it to ForkJava/LocalJava? If so then I think as late as possible is better.

@dwijnand By moving the logging to ForkJava/LocalJava, we can only fix #2087 : we're forking and we log the options that are used.
#2041 needs to be done upfront, since we're not forking.

@jsuereth @dwijnand I just pushed a fix for the bincompat issue, by addind an overload for allGroupTestTasks.

Doesn't LocalJava deal with the non-forked version?

LocalJava seems to deal only with compilation :/

More precisely, LocalJava goal (as stated by its documentation : Helper methods for trying to run the java toolchain out of our own classloaders.) is to run the Javadoc tool or javac in process.
After all, running a main class or tests in process boils down to calling a method.

BTW, what @Duhemm has done on his fork does not affects the run or test tasks, as what he has done logs the JVM options used for javac/javadoc.

BC changes look good, was just commenting on what the error messages look like in practice (From a quick test).

Ideally I'd like to know, if I set fork in (Compile, run) := false and javaOptions in Compile := Seq(...) I'd like to know WHICH runner is the problematic one.

Indeed, as it is, It's nothing more than a hint that something is wrong for any not-completely-trivial build.
Will fix ASAP.

I have two small questions:

• if we are to print the configuration, this mean that we need to add yet another parameter to allGroupTestTasks, which is not so hard in itself. However, as we'll need to provide a default value for that parameter, which configuration should be used as the default value ?
• Is there a simple way to get the name of the Scope ? I found Scope.display, should I use it ?

So, as for allGroupTestTasks... hmm... Maybe let's defer that for now. I think there may be an alternative way to get the error message on that setting.

As for Scope.display, that should be fine. I personally get confused over the different show options EVERY TIME I need to show something, so you should try it out and check the error messages. if it's wrong, we'll find you the right one :)

Also, thanks @pdalpra for the dedication on this one. Very glad to see you going the extra mile on error messages :)

As shown above, I managed to get something that looks quite good, except for the scopes...
Is javaOptions.scopedKey the right way to get the key with its scope ?

You're welcome :)
After all, if we're going to add log messages, let's do it right, no ? ;)

no, I dont think javaOptions.scopedKey is doing the right thing...

It seems that what is done in SettingsGraph (used by inspect tree) could get me to the info I want, but it looks like it's overkill just to get scoped run and javaOptions :/

@pdalpra I'll have to look. The issue is NOT that you want the scopedKey of the current key, it's that you want to find the scope of the current Setting you're a part of.

SO, for ANY key, you can do: key.scopedKey. This is because, internally, sbt uses ScopedKeys for its setting system. A scoped key is just an AttributeKey and a Scope. TaskKey and SettingKey and friends are the user-facing DSL for these. They allow you to manipulate them and also keep the high level API for things.

If you just call javaOptions.scopedKey you'll get a ScopeKey with the default scope. If you're inspecting a setting AFTER setting resolution, you'll see the real scope for the key (we do translation/resolution during a build-compile step).

SO yes, SettingGraph is looking at the Finalized ScopedKey (our internal API). What you see in your setting is before this step, so the scopedKey would be wrong.

WHat we do have in sbt, are a few "magic" keys that will be injected by the setting system. For example, taskStreams is a setting which gets injected based on the scope of the key the setting is being applied to. We expose ONE of these magic keys called "configuration" which you can use to grab the current configuration you're being applied to. See: https://github.com/sbt/sbt/blob/0.13/main/src/main/scala/sbt/Load.scala#L142-L168

TL;DR; Try to depend on resolvedScope key and that should give you the right scope.

Thank you for your detailed explanation, I'll try it ASAP.

Got it :

[warn] {file:/Users/pdalpra/test/}test/compile:run::javaOptions will be ignored, {file:/Users/pdalpra/test/}test/compile:run::fork is set to false

I only logged scopes on the warning, not on the debug that display the javaOptions.

Can't get that level of details to allGroupTestTasks however.

Amended my commit so that the project axis in not shown in the warning message :

[warn] compile:run::javaOptions will be ignored, compile:run::fork is set to false

@pdalpra nice!

Nice indeed!

BTW, thank you for your patience with me on this @jsuereth !