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

Don't use the MSBuild server process for init-build.proj. #14643

Merged
merged 2 commits into from
Oct 11, 2022

Conversation

crummel
Copy link

@crummel crummel commented Oct 3, 2022

This appears to fix an issue where our SdkResolver is not loaded because there are still lingering dotnet processes when we try to binplace the resolver DLL.

Fixes dotnet/source-build#3021.

This appears to fix an issue where our SdkResolver is not loaded because there are still lingering dotnet processes when we try to binplace the resolver DLL.
@crummel crummel requested a review from a team as a code owner October 3, 2022 15:57
@baronfel
Copy link
Member

baronfel commented Oct 3, 2022

You should also be able to shutdown any persistent servers (MSBuild or compiler) with dotnet build-server shutdown immediately prior to any command that needs access to any potentially-loaded files.

@MichaelSimons
Copy link
Member

@baronfel, Do you know if there was any behavior change in this space? This code hasn't changed since the beginning of 7.0 and recently broke. Is it expected to have to explicitly shutdown the build server to get new SdkResolvers to load?

@baronfel
Copy link
Member

baronfel commented Oct 3, 2022

Yeah - in 7.0 rc1 MSBuild server was enabled by default, which is why I think you're seeing this behavior now. From that time we've squashed a few bug in it, but we are still deciding if the feature should be on for the release. I'm not sure if I'd classify this specific interaction as a bug - for that I'll tag @rokonec for his opinion.

@MichaelSimons
Copy link
Member

@baronfel - Thanks for the explanation.

@crummel - Thoughts on using dotnet build-server shutdown?

@crummel
Copy link
Author

crummel commented Oct 4, 2022

I think using the shutdown command is better as it's more clear in the intent. I've changed the PR to use that and added a comment about what we're trying to do.

@rokonec
Copy link

rokonec commented Oct 4, 2022

I would very like to understand why is lingering dotnet processes of MSBuild server an issue for you? Can you please explain...

@MichaelSimons
Copy link
Member

@crummel - you mentioned the MSBuild server is going to be disabled by default for 7.0 and this change would not be required. Can you link to the corresponding msbuild PR issue discussing this?

@baronfel
Copy link
Member

baronfel commented Oct 7, 2022

Saving some time since I saw this pop up: dotnet/sdk#28369 is the PR to change the default behavior to be an opt-in instead of an opt out. we got tactics approval yesterday so should merge today.

@MichaelSimons
Copy link
Member

rokonec - Can you answer the question if it is expected to have to explicitly shutdown the build server to get new SdkResolvers to load? We are trying to make an informed decision if we should proceed with this PR even though the default behavior is being changed to opt-in?

@rokonec
Copy link

rokonec commented Oct 11, 2022

@MichaelSimons I believe shutdown servers is safer approach. It will still work if in future server will be on by default.
Additionally it will shutdown all msbuild worker nodes processes which could also cause stale-cache-issues in some cases.

@crummel crummel merged commit c516635 into dotnet:release/7.0.1xx Oct 11, 2022
MichaelSimons added a commit that referenced this pull request Nov 14, 2022
* Adding ppc64le arch for source build (#14631)

* Remove llvm-project from source-build as we have removed this dependency in runtime. (#14697)

* Remove llvm-project from source-build as we have removed this dependency in runtime.

* Add patch for the backport PR to get this to build for now.

* Remove unneeded copy of this patch.

* [release/7.0.1xx] Update dependencies from dotnet/sdk (#14705)

[release/7.0.1xx] Update dependencies from dotnet/sdk


 - Remove backported command-line-api patch

* Don't use the MSBuild server process for init-build.proj. (#14643)

* Don't use the MSBuild server process for init-build.proj.
This appears to fix an issue where our SdkResolver is not loaded because there are still lingering dotnet processes when we try to binplace the resolver DLL.

* Address code review feedback.

* Update patch backport comments and remove obsolete patch (#14710)

* [release/7.0.1xx] .NET Source-Build 7.0.100-rc.2 October 2022 Updates (#14728)

* Update source-build to 7.0 RC2

* Also update tarball global.json

* Don't touch root global.json in case it is managed by automation

* Add back source-build patch that is still required in this branch

Co-authored-by: Swapnali911 <Swapnali.Pawar1@ibm.com>
Co-authored-by: Chris Rummel <crummel@microsoft.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Michael Simons <msimons@microsoft.com>
Co-authored-by: Logan Bussell <loganbussell@microsoft.com>
Co-authored-by: Jason Zhai <v-wuzhai@microsoft.com>
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.

4 participants