Flip meaning of AllowFailureWithoutError Fixes #5701

[Forgind]

Fixes #5701.

Changes its meaning without altering the default behavior. Also fixed tests and added one to verify that the default is as expected.

[rainersigwald]

LGTM but let's talk this over in triage tomorrow to make sure we're all on board with the "breaking change" (to do what it says it would do).

[Forgind]

@sae42,

Since you're the only user of this that we know of, FYI and do you think this change would be too annoying?

[sae42]

@Forgind Intuitively this feels like the right thing to do and better to fix now. The only issue I can see (and it may not be a big issue) is the value assigned to this property (to avoid MSB4181 being output) would be false in 16.7 and true for 16.8+. Is there a really easy way to determine if a task is running under 16.7 or 16.8 etc? As a aside, when will the MSBuild NuGet packages be updated? Right now they are 16.6 so the only way to use IBuildEngine7 is using the MSBuild assemblies in a VS install or use the reflection hack.

[Forgind]

We came up with three ways, although two are pretty bad:

1. Use the BuildEngine to build a small project that outputs the version.
2. Reflect on an MSBuild type to find the assembly and then its version.
3. Pass $(MSBuildVersion) as a parameter to the task.

I'm also a little worried about users using an early 16.8 preview, since just looking at the 16.8 part of the version would you make you think this boolean is flipped, but it isn't. That's only a short-term problem, though, whereas getting it wrong on 16.7 is a long-term problem.

The new ones should have already been pushed, but they weren't for some reason. I reported it to the relevant people, so it should be reasonably soon now. Sorry about that!

[rainersigwald]

• Use the BuildEngine to build a small project that outputs the version.

Definitely do not do this.

[sae42]

Actually, they all sound pretty bad to me. Perhaps a future API enhancement in the build engine to get a version would be a useful feature.
I think I would be happy with treating 16.7 as a dead/unsupported version. 16.8 is LTS and I suspect any users on 16.7 would swiftly move to 16.8 when available anyway. So I'll align with that.

[Forgind]

16.7 and 16.9 are LTS, unfortunately. That's only a few extra months, but it's still problematic.

[sae42]

16.7 and 16.9 are LTS, unfortunately. That's only a few extra months, but it's still problematic.

Ah, you're right - that's a pain. What are the timescales for 16.9 - could you delay this change until then?

If the balance of opinion says make the change now, I'd say go with it and I'll figure out a way to live with it (probably option 2 above)

[benvillalobos]

Team Triage: We're going to wait for 16.9

[nohwnd]

@Forgind VSTest also uses this. For the net5.0 release I flipped the switch in our code as suggested by @rainersigwald to get rid of the error message. Any suggestion of what to do next? Will you be adding and API to check the version? We insert VStest into VS and dotnet SDK, but there is also TestPlatform pacakge, and integration with VSCode which looks up available MSBuild versions, so I don't think we can just flip the default in the new version and call it a day. 🙁

[Forgind]

@nohwnd, do any of the options in #5743 (comment) work for you?

What version of MSBuild does net5.0 use?

[nohwnd]

2. should work, we are already using reflection to set the option anyway. I think we build against 15.0 but use whatever is available in the place where we insert, which would be VS, and 3.1 and 5.0 SDK.

I don't know why it did not click before, thanks :)