Correct success for /preprocess /targets builds #8908
Conversation
This is not yet a problem but only because neither /preprocess nor /targets are supported for solution files. The root of the problem is if someone chooses to specify both /preprocess and /targets. If /preprocess fails but /targets succeeds, it currently will erroneously display success. This fixes that. As I said, that scenario doesn't currently exist but only because /targets cannot succeed unless /preprocess succeeded, but that is not guaranteed going forward. Notably, if /preprocess is extended to support solution files before /targets is, this will become an issue.
There was a problem hiding this comment.
I still think the combination of -preprocess and -targets makes no sense and should be forbidden.
If it is allowed, I don't understand why we should attempt to print targets after preprocessing has failed--what's the scenario there?
@jrdodds commented just under your comment that the two operations are orthogonal and accept files in which to put the outputs. I'm inclined to agree with him. Is your concern here that you don't see a scenario in which someone could want to do this, or is there some reason you think it would actively hurt customers if they try to do it?
Slight misunderstanding here—if preprocessing fails, we currently do continue on and still (if requested) try to print targets. I think there's an argument that if we fail to preprocess the project file, we should skip printing targets, but I'm not concerned with that issue in this PR. What does concern me is if we fail to preprocess then go on to try to create a list of targets and succeed for some reason because then the build result would be "success," and the user would be very confused. |
|
I think it is nonsensical to use the "instead of build, preprocess the file" and "instead of build, list the targets in the file" options together.
Yes--I'm saying that is the bug. |
|
Changing line 1285 to: if (isTargets && success)
{(i.e. stopping on a preprocess error regardless of what may follow) would make sense to me. |
|
@Forgind is this ready to go? |
I think so, yeah. Just not very high priority 🙂 |
Forgind
added
the
merge-when-branch-open
* Correct success This is not yet a problem but only because neither /preprocess nor /targets are supported for solution files. The root of the problem is if someone chooses to specify both /preprocess and /targets. If /preprocess fails but /targets succeeds, it currently will erroneously display success. This fixes that. As I said, that scenario doesn't currently exist but only because /targets cannot succeed unless /preprocess succeeded, but that is not guaranteed going forward. Notably, if /preprocess is extended to support solution files before /targets is, this will become an issue. * Make isTargets not run if !success --------- Co-authored-by: Forgind <Forgind@users.noreply.github.com>
* Correct success This is not yet a problem but only because neither /preprocess nor /targets are supported for solution files. The root of the problem is if someone chooses to specify both /preprocess and /targets. If /preprocess fails but /targets succeeds, it currently will erroneously display success. This fixes that. As I said, that scenario doesn't currently exist but only because /targets cannot succeed unless /preprocess succeeded, but that is not guaranteed going forward. Notably, if /preprocess is extended to support solution files before /targets is, this will become an issue. * Make isTargets not run if !success --------- Co-authored-by: Forgind <Forgind@users.noreply.github.com>
This is not yet a problem but only because neither /preprocess nor /targets are supported for solution files.
The root of the problem is if someone chooses to specify both /preprocess and /targets. If /preprocess fails but /targets succeeds, it currently will erroneously display success. This fixes that.
As I said, that scenario doesn't currently exist but only because /targets cannot succeed unless /preprocess succeeded, but that is not guaranteed going forward. Notably, if /preprocess is extended to support solution files before /targets is, this will become an issue.