Skip to content

Conversation

benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented Apr 19, 2021

Fixes: #16818

This prevents more probing paths being added when not necessary.

Changes Made

The GenerateRuntimeConfigurationFiles class already checks to see if it should generate a .dev file. When the value is null or empty it doesn't.

The fix was adding a property and a condition to set the value of RuntimeConfigDevPath.

No value on RuntimeConfigDevPath means no *.runtimeconfig.dev.json file should be generated. I considered conditioning the target on the property, but that target also generates *.runtimeconfig.json, and I'm not sure if we want to keep that or not.

Notes

Also not sure if we wanted to explicitly define the property as false, but I added the property along with context to Microsoft.NET.Sdk.props.

This prevents more probing paths being added when not necessary.
@ghost
Copy link

ghost commented Apr 19, 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.

@mateoatr
Copy link
Contributor

Could we also add a test for this? Just to make sure that the runtime.deps.json is generated when using the property and also that by default we no longer generate it.

@benvillalobos
Copy link
Member Author

@mateoatr sure, could you point me in the right direction for where these tests should go? Quick searches point me to GivenAGenerateRuntimeConfigurationFiles but those look like they run based off of runtimeconfig.dev.json files that are already given.

@mateoatr
Copy link
Contributor

I'd think it could live in a GivenThatWeWantToBuild____, although not sure, maybe @dsplaisted has a better suggestion?

@dsplaisted
Copy link
Member

The test should go in the Microsoft.NET.Build.Tests project. I don't know if there's an existing test class that it should go in, it can also go in a new one.

@benvillalobos
Copy link
Member Author

So I created a test that should determine whether the runtimeconfig.dev.json file gets generated when building an sdk project.

I'm running into a snag, it looks like the dev.json file is still being generated for net6.0. My first assumption is that the test isn't actually using an updated Sdk.props file, but I haven't verified that yet. Is there anything special that needs to be done to do so? If not, it's likely my logic.

@marcpopMSFT
Copy link
Member

FYI @brianrob for visibility in your perf tracking. This should get in for preview 5.

[InlineData("netcoreapp3.0", true)]
[InlineData("net5.0", true)]
[InlineData("net6.0", false)]
public void It_stops_generating_runtimeconfig_dev_json_after_net6(string targetFramework, bool generateRuntimeConfigDevJson)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also check that the file is indeed generated if ("net6.0", true) and that using the new property GenerateRuntimeConfigDevFile has no effect on previews versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

This helped me realize I needed to only set the property if it wasn't already set, thanks for that. Added more tests as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized I implemented it so the user-set property has the final say. You're saying no matter what, if the user sets GenerateRuntimeConfigDevFile to true and targets < net6.0, we should still generate the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay. GenerateRuntimeConfigDevFile should be false by default for .NET 6 onwards, and true otherwise. All previous versions should be generating this file by default. I think it's fine if we allow previous versions to stop generating the file whenever a user sets <GenerateRuntimeConfigDevFile>false</GenerateRuntimeConfigDevFile>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect. The change is good to merge then 👍

Copy link
Contributor

@mateoatr mateoatr left a comment

Choose a reason for hiding this comment

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

Other than adding a few more test cases, LGTM. Thanks!

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.

Stop adding a runtimeconfig.dev.json by default on built apps
4 participants