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

Update Traits when environment has been changed #9655

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

AR-May
Copy link
Member

@AR-May AR-May commented Jan 17, 2024

Fixes #9501

Context

When the new environment is sent to msbuild node or msbuild server node, the environment is set up but the values in the Traits class are not updated. This leads to using a different configuration on the server or MSBuild node than on the main MSBuild node when the server or msbuild node was preserved from the previous builds.

Changes Made

Together with setting the new environment, re-create a Traits class instance to update the corresponding values.
I bit of refactoring: there was a pattern in the code "unset current environment and set a new one", which is repeated in many places.

Testing

Manual tests, unit tests, exp VS insertion

Notes

This change is rather a work-around for the bigger problem of handling the environment properly in MSBuild nodes: the configuration sometimes is taken from the Traits and sometimes directly from environment variable. Also, as in this issue, sometimes code sets new environment variables but does not update the values in Traits (and sometimes it updates). We might consider fixing how we deal with configuration.

@AR-May AR-May requested a review from JanKrivanek January 17, 2024 16:23
@JanKrivanek
Copy link
Member

Overall looks good - though the pattern "clear pre-existing env variables not present in the Dictionary; set the env vars from the dictionary; refresh the Traits" is on 3 different places - so it would be nice to extract single utility class and call it.

(I know it's pre-existing code - but this is good opportunity to make it better)

@AR-May
Copy link
Member Author

AR-May commented Jan 17, 2024

Overall looks good - though the pattern "clear pre-existing env variables not present in the Dictionary; set the env vars from the dictionary; refresh the Traits" is on 3 different places - so it would be nice to extract single utility class and call it.

(I know it's pre-existing code - but this is good opportunity to make it better)

I am considering doing that and place this code in CommunicationsUtilities indeed. First will wait for tests passing though, do not want spend time on that if this approach will be wrong.

@AR-May
Copy link
Member Author

AR-May commented Jan 19, 2024

The tests seem pass: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/522067. Refactored the code a bit and making this draft ready to review.

@AR-May AR-May marked this pull request as ready for review January 19, 2024 12:03
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good!

src/Build/BackEnd/Node/OutOfProcNode.cs Show resolved Hide resolved
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

The changes seem reasonable. The test failures all seem to be on tests that apparently sometimes flakily fail, but I wonder if this made them more consistently fail on windows for some reason. ProjectStarted has no properties?

@AR-May
Copy link
Member Author

AR-May commented Jan 22, 2024

The changes seem reasonable. The test failures all seem to be on tests that apparently sometimes flakily fail, but I wonder if this made them more consistently fail on windows for some reason. ProjectStarted has no properties?

Apparently, I broke a test with my refactoring. There was one place where code unsets variables a bit differently than in CommunicationsUtilities.SetEnvironment and it matters (I thought initially it would not). Had to revert one change to pass tests.

@AR-May AR-May requested review from ladipro and rokonec and removed request for ladipro January 24, 2024 15:10
src/Shared/CommunicationsUtilities.cs Outdated Show resolved Hide resolved
@AR-May AR-May merged commit caaccdd into dotnet:main Jan 30, 2024
8 checks passed
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.

ProjectImportedEventArgs missing from most projects
4 participants