Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed /restore and /graph conjunction error on exit code #9461

Merged
merged 9 commits into from
Dec 8, 2023
Merged

Conversation

maridematte
Copy link
Contributor

@maridematte maridematte commented Nov 27, 2023

Fixes #9443

Context

When the /graph and /restore are used within the same command, the build exit code will always be 0. This is because of the variable used to define success of the restore action overrides the success of the graph build.

Changes Made

Added an extra condition when defining the success of the build to account for this case.

Testing

Made sure existing tests passed and added a unit test for this case, an some manual testing.

@maridematte maridematte marked this pull request as ready for review November 28, 2023 12:33
@maridematte maridematte changed the title Fixed /restore and /graph conjunction error Fixed /restore and /graph conjunction errorcde Nov 28, 2023
@maridematte maridematte changed the title Fixed /restore and /graph conjunction errorcde Fixed /restore and /graph conjunction error on exit code Nov 29, 2023
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it would be slightly better if the success = assignments were done right after the respective results are calculated. The current code effectively repeats the condition graphResult != null && !saveProjectResult in two places in an non-obvious way and it would be easy to have them diverge and introduce a similar bug in the future. Especially given what a monster of a method this is.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the refactoring a lot!

src/MSBuild.UnitTests/XMake_Tests.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
@AR-May
Copy link
Member

AR-May commented Dec 6, 2023

I think we should target vs17.9 branch for this PR, since this is a bug?

@maridematte maridematte changed the base branch from main to vs17.9 December 6, 2023 14:51
src/MSBuild.UnitTests/XMake_Tests.cs Outdated Show resolved Hide resolved
@maridematte maridematte merged commit c70267d into vs17.9 Dec 8, 2023
8 checks passed
@maridematte maridematte deleted the 9443 branch December 8, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: When using /graph the exit code is always 0 even on a build failure MSBuild version 17.8.3+195e7f5a3
4 participants