Skip to content

Disable support for MetadataUpdate (hot reload) in non-Debug builds. #18810

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.

Sign up for GitHub

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

Jump to bottom
Merged
pranavkm merged 3 commits into from
Jul 9, 2021
Merged

Disable support for MetadataUpdate (hot reload) in non-Debug builds. #18810

pranavkm merged 3 commits into from
Jul 9, 2021

Conversation

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jul 8, 2021

No description provided.

@ghost
Copy link

ghost commented Jul 8, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@pranavkm pranavkm requested review from eerhardt and sbomer July 8, 2021 18:55
sbomer
sbomer reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Would it make sense to put this in https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props?

@eerhardt
Copy link
Member

eerhardt commented Jul 8, 2021

In general, .props files are for defaulting properties that we expect the .csproj to read, and .targets are for defaulting properties that can be set in the .csproj.

So in this case, the .targets probably makes the most sense. A .csproj doesn't need to read this property.

eerhardt
eerhardt reviewed Jul 8, 2021
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets Outdated
Value="$(ThreadPoolMaxThreads)" />
</ItemGroup>

<PropertyGroup>
Copy link
Member

@eerhardt eerhardt Jul 8, 2021

Choose a reason for hiding this comment

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

This feels like an odd spot for this property group. Would it make sense to go higher in the file, before any <Targets>? Like maybe around line 100?

Copy link
Member

@eerhardt eerhardt Jul 8, 2021

Choose a reason for hiding this comment

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

Thinking a bit more, should this switch get set for PublishTrimmed=true instead of non-Debug configurations? Does it have any effect on non-trimmed applications?

Copy link
Member

@eerhardt eerhardt Jul 9, 2021

Choose a reason for hiding this comment

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

@pranavkm explained to me that setting this for PublishTrimmed=true would be a bad idea because then it would be disabled even during dotnet run and when debugging. So I realize now that was not a good idea.

@pranavkm
Copy link
Contributor Author

pranavkm commented Jul 8, 2021

Would it make sense to put this in

It's also important because putting things in targets lets you make decisions based on what a user configured in their project file. Props file is imported before a project, so you can't do that.

@eerhardt updated to add a test.

eerhardt
eerhardt approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks!

@pranavkm pranavkm enabled auto-merge (squash) July 9, 2021 16:51
@pranavkm pranavkm mentioned this pull request Jul 9, 2021
Merged
eerhardt
eerhardt reviewed Jul 9, 2021
View reviewed changes
src/Tests/Microsoft.NET.Build.Tests/MetadataUpdaterSupportTest.cs
{
}

[CoreMSBuildOnlyFact] // Running on desktop causes failures attempting to restore M.NETCore.App.WinHost.
Copy link
Member

@eerhardt eerhardt Jul 9, 2021

Choose a reason for hiding this comment

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

That seems like a bug somewhere.... This is a super simple test....

cc @dsplaisted

Copy link
Contributor Author

@pranavkm pranavkm Jul 9, 2021

Choose a reason for hiding this comment

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

I was misled by the error messages. It looked like it was restore failures, but it looks like it's because of the new compiler feature that needs a newer VS on the Helix machine:

"C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj" (Build target) (1:7) ->
        (CoreCompile target) -> 
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(2,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(3,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(4,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(5,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(6,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(7,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
          C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\obj\Debug\net6.0\HelloWorld.ImplicitNamespaceImports.cs(8,1): error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [C:\h\w\B468094B\t\dotnetSdkTests\dft2qt2w.g3m\It_Configures---A34EAFFF\HelloWorld.csproj]
        
            0 Warning(s)
            7 Error(s)

Copy link
Member

@eerhardt eerhardt Jul 9, 2021

Choose a reason for hiding this comment

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

ouch. Can you open an issue for this?

Copy link
Member

@dsplaisted dsplaisted Jul 9, 2021

Choose a reason for hiding this comment

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

You should be able to use [RequiresMSBuildVersionFact("17.0.0.32901")] for this, and it will skip the test when running on the older MSBuild, but automatically start to run again on Full Framework once CI is updated to a newer VS.

@pranavkm @eerhardt

eerhardt
eerhardt reviewed Jul 9, 2021
View reviewed changes
src/Tests/Microsoft.NET.Build.Tests/MetadataUpdaterSupportTest.cs
var testAsset = _testAssetsManager
.CopyTestAsset("HelloWorld")
.WithSource()
.WithTargetFramework("net6.0");
Copy link
Member

@eerhardt eerhardt Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
.WithTargetFramework("net6.0");
.WithTargetFramework(targetFramework);

eerhardt
eerhardt reviewed Jul 9, 2021
View reviewed changes
src/Tests/Microsoft.NET.Build.Tests/MetadataUpdaterSupportTest.cs
var testAsset = _testAssetsManager
.CopyTestAsset("HelloWorld")
.WithSource()
.WithTargetFramework("net6.0");
Copy link
Member

@eerhardt eerhardt Jul 9, 2021

Choose a reason for hiding this comment

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

Suggested change
.WithTargetFramework("net6.0");
.WithTargetFramework(targetFramework);

@pranavkm pranavkm merged commit 282e740 into main Jul 9, 2021
@eerhardt eerhardt deleted the prkrishn/metadata-updater-support branch July 9, 2021 17:54
@eerhardt eerhardt mentioned this pull request Aug 11, 2021
Merged
@MichalStrehovsky MichalStrehovsky mentioned this pull request Jan 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsplaisted dsplaisted dsplaisted left review comments

@eerhardt eerhardt eerhardt approved these changes

@sbomer sbomer sbomer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pranavkm @eerhardt @dsplaisted @sbomer