-getTargetResult + -target behavior is unexpected

[rainersigwald]

-getTargetResult will run a target that otherwise would not run.

E.g. Given a project with targets one and two and the targets have no dependency between them:

<Project>
  <Target Name="One">
    <PropertyGroup>
      <First Condition="$(First) == ''">One</First>
    </PropertyGroup>
    <Message Text="One" />
    <Message Text="First = $(First)" />
  </Target>

  <Target Name="Two">
    <PropertyGroup>
      <First Condition="$(First) == ''">Two</First>
    </PropertyGroup>
    <Message Text="Two" />
    <Message Text="First = $(First)" />
  </Target>
</Project>

and given a command line with -target:one -getTargetResult:two, both targets will be executed.

The current behavior executes the targets provided to -getTargetResult that didn't already execute, after the 'standard' target build order. Essentially there is a secondary chain of targets.

It seems from the discussion that this is not the intended behavior.

My own quick take is that -getTargetResult should not itself cause a target to be executed and if a target provided to -getTargetResult didn't execute then there is no result to report.

Originally posted by @jrdodds in #3911 (comment)

[rainersigwald]

I think we should disallow -target -getTargetResults. IMO, -getTargetResults should specify entrypoint targets, and allowing two ways to specify that leads to confusion.

[jrdodds]

-target already exists and already specifies the entry point targets. Changing -target could create issues. Perhaps -getTargetResults (or a renamed switch) should be a Boolean flag where 'true' indicates that the results of the entry point targets in -target should be reported. But this couples the entry point targets and the result reporting. Are there scenarios where the results are not desired for all entry points targets and/or results are desired for a target deeper in the target build order?

Myself I think that limiting results reporting to just the entry point targets is too limiting.

[Forgind]

This may have been a part of a design discussion that wasn't fully fleshed out, but having -getTargetResult add the targets it wants to execute to the list of targets that need to be executed was how I'd intended for it to work. We can't return results from targets that don't execute, so we clearly have to include targets listed in -getTargetResult. If you request more targets via -target, I think that should be permitted because otherwise, you wouldn't be able to ask for the result of Two in the context of One having been executed. (Like imagine if One doesn't have any connection to Two as far as MSBuild is concerned but does change some files on disk...well, then it may indirectly affect Two, and we'd need a way to say "I want One and Two to execute, but I only care about the result from Two.")

So my personal take is that this is exactly the desired behavior, but I'll @baronfel for a more official opinion.

[rainersigwald]

we'd need a way to say "I want One and Two to execute, but I only care about the result from Two."

Isn't that -getTargetResults:One;Two, and then don't actually look at the results of One?

[Forgind]

That's something you can do, and in your example, it would be pretty reasonable. One could potentially have an enormous list of results, though, and many users are very concerned with not having excess information thrown around. If they want everything, they can just look at a binlog. Having the two flags separate permits more precision on the part of the user.

[jrdodds]

I'm in favor of not coupling the "entry point" targets and the "result" targets.

I'm not in favor of having two switches define the "entry point" targets.

It does appear that the targets in -getTargetResult are included in determining the target build order but, unless otherwise specified by dependencies, all targets in -getTargetResult are executed last. That is not self-evident and would need to be documented.

We can't return results from targets that don't execute, so we clearly have to include targets listed in -getTargetResult.

I'm not on board with the assumption that every target in -getTargetResult must report results. In the output JSON, "TargetResults" can be empty.

{
  "TargetResults": {
  }
}

This could be a quick troubleshooting technique without checking logs or adding Message. If there is no result, then the target is not being executed.

I think it is reasonable and a better separation of responsibilities, that -target defines the "entry point" targets and -getTargetResult defines the 'allow' list of targets that if executed report results.

[Forgind]

@baronfel, I think this fell through the cracks at some point. I still think the current design is the best option available, but since there's some disagreement on that point, it'd be good to get some resolution.

My reasons:

• If we don't have -getTargetResult add its targets to -targets, then we might not have the relevant targets execute, in which case the target results will be uninteresting. Since the user specifically requested the result from that target, we should make sure it executes, i.e., by adding it to -targets.
• If we disallow specifying both -targets and -getTargetResults, then someone who already has -targets as part of their build script can't use -getTargetResults without first modifying their build script, which would be a pain.
• If we make -getTargetResult take priority over -targets (basically clear it out—I didn't see this proposed, but I could make an argument for it; I just think it's a bit weak) then users wouldn't be able to see what the result of executing target Foo is in the context of Bar also having been executed.

It does appear that the targets in -getTargetResult are included in determining the target build order but, unless otherwise specified by dependencies, all targets in -getTargetResult are executed last. That is not self-evident and would need to be documented.

This is sorta true but more incidentally than intentionally. That's often the result of specifying that a particular target should build: it builds all the things it depends on before it itself builds. If you have AfterTargets="Foo", though, that target will build after any targets you specify. They just shouldn't affect the results.

This could be a quick troubleshooting technique without checking logs or adding Message. If there is no result, then the target is not being executed.

I think this is a very good point. It raises the question as to whether people are more likely to be surprised by an empty target result (and probably think the feature isn't working as a result) or unhappy because there's no easy way to check whether a target should run. I don't know the answer to that. My personal take is that the former is more important because if I want to know whether a target ran or not, once I have the answer, my next question is 'why', and you wouldn't get that out of this; you'd have to look at logs. But that's just how I would go about it, so I'm curious to hear what @baronfel thinks.