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

Stop adding a runtimeconfig.dev.json by default on built apps #16818

Closed
mateoatr opened this issue Apr 9, 2021 · 10 comments · Fixed by #17014
Closed

Stop adding a runtimeconfig.dev.json by default on built apps #16818

mateoatr opened this issue Apr 9, 2021 · 10 comments · Fixed by #17014
Assignees
Milestone

Comments

@mateoatr
Copy link
Contributor

mateoatr commented Apr 9, 2021

Probing consumes a significant ammount of time at startup in the host. With changes introduced in dotnet/runtime#50671, we move away from expensive file-accessing calls made by the host's probing logic and make them conditional upon whether any additional probing paths were passed to hostpolicy. This improves performance of any dotnet ... command as well as the time to main.

Note that by default the SDK generates a <app>runtimeconfig.dev.json when building an app, which ends up adding additional probing paths which are not necessary (all dependencies are included in the app folder which is probed first, related: #15210) -- causing the host to probe more than necessary and missing on any performance gains achieved by the above PR.

/cc @vitek-karas @brianrob

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Apr 9, 2021
@dotnet-issue-labeler
Copy link

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

@vitek-karas
Copy link
Member

@dsplaisted @wli3 What do you think?

The intent is to introduce a new MSBuild property which would be off by default and which would determine if the .runtimeconfig.dev.json should be generated or not.

@dsplaisted
Copy link
Member

Yes, I think we should turn of the runtimeconfig.dev.json by default. I think we still need it prior to 3.x where we wouldn't copy referenced NuGet DLLs to the output folder. Also we may still need it if we are using the runtime package store for this line: "C:\\Users\\daplaist\\.dotnet\\store\\|arch|\\|tfm|",

@vitek-karas
Copy link
Member

@dsplaisted If I understand the "package store" correctly, that line is only needed if I used dotnet store locally and want to test my apps against it, right? We will not produce this into a published app ever. Also, there's no way to detect in the SDK today that the app will want to use package store, right?

The proposed changes should be for TFM .net6 and above only (the hosting changes will be there as well). I personally would not mind too much if using the package store locally would require modifying the csproj of the tested app, or manually adding the file there.

@marcpopMSFT marcpopMSFT self-assigned this Apr 13, 2021
@dsplaisted
Copy link
Member

@normj Do you know if the runtimeconfig.dev.json file is important for AWS lambda scenarios using the runtime package store?

@benvillalobos
Copy link
Member

Sounds like we want to stop this from happening by default while having some flag if you need this to be generated.

@brianrob
Copy link
Member

We also want to make sure that dotnet.runtimeconfig.dev.json doesn't get emitted. From a quick look at a binlog for the SDK build, it looks like fixing this issue and then consuming the new SDK should address this.

@benvillalobos
Copy link
Member

Looks like it's already set to avoid creating the file if it's passed an empty ProjectRuntimeConfigDevFilePath.

GenerateRuntimeConfigurationFiles.cs

        protected override void ExecuteCore()
        {
            bool writeDevRuntimeConfig = !string.IsNullOrEmpty(RuntimeConfigDevPath);

Current thought is to add a condition to setting ProjectRuntimeConfigDevFilePath so that it fails to set that value unless the property is set.

The intent is to introduce a new MSBuild property which would be off by default and which would determine if the .runtimeconfig.dev.json should be generated or not.

The first time I read this, I understood it as "create the property in the msbuild repo", but after looking at this for a bit I don't agree with that assumption. This should be defined in the SDK repo under some sdk.props file right?

@benvillalobos
Copy link
Member

*.runtimeconfig.dev.json is generated at the same time as *.runtimeconfig.json, presumably we want to keep the latter?

@vitek-karas
Copy link
Member

The first time I read this, I understood it as "create the property in the msbuild repo", but after looking at this for a bit I don't agree with that assumption. This should be defined in the SDK repo under some sdk.props file right?

Sorry for the confusion. I'm using the term "MSBuild property" as a way to describe that we define a property name in our props/targets which users can set to change the behavior (to avoid confusion with .NET API property). I definitely think that this should be simply defined in the SDK. I personally would prefer a "boolean-like" property, to get back the current behavior (having to specify a path makes it a bit cumbersome).

We do need to keep .runtimeconfig.json - absolutely.

@marcpopMSFT marcpopMSFT added Area-NetSDK and removed untriaged Request triage from a team member labels Apr 21, 2021
@marcpopMSFT marcpopMSFT added this to the 6.0.1xx milestone Apr 21, 2021
pregress added a commit to pregress/service-fabric-services-and-actors-dotnet that referenced this issue Nov 12, 2021
.NET 6 doesn't create runtimeconfig.dev.json by default. See:  dotnet/sdk#16818
PavanKunapareddyMSFT pushed a commit to microsoft/service-fabric-services-and-actors-dotnet that referenced this issue Nov 19, 2021
.NET 6 doesn't create runtimeconfig.dev.json by default. See:  dotnet/sdk#16818
pakunapamsft added a commit to microsoft/service-fabric-services-and-actors-dotnet that referenced this issue Feb 24, 2022
* LocalStoreConfigFix (#288)

* Remove myget feed

* Bumpup build version to consume latest bug fixes

* Fix missing dependency upgrade

* Check in runtimeconfig.dev.json exists (#289)

.NET 6 doesn't create runtimeconfig.dev.json by default. See:  dotnet/sdk#16818

* Bumpup build version to consume latest bug fixes

* Modifying actor enumeration for KVS (#295)

* Modifying actor enumeration for KVS

* Addressing comments

* Minor change to prevent additional loop during actor enumeration

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Bump up build version to consume latest bug fixes

* Load KeyValueStore settings from settings.xml (#292)

* Bumpup build version to consume latest fixes

* Enumerate Reminders (#298)

* Add GetRemindersAsync state provider api

* Add GetReminders support in actor manager

* Buf fixes

* Bug fixes

* Build fix

* Enumerator dispose in KVS getactorsasync

* Bumpup build version to consume latest fixes

* V2 PagedResult fix (#300)

* V2 PagedResult fix

* Create ReminderPagedResult to serve GetRemindersAsync call

* Bump up build version to consume latest bug fixes

* Remoting exception serialization  (#296)

* Add Service convertors

* Add client convertors

* Fix build issues

* Add actor convertors

* Add FabricExceptionConvertor test

* Add test cases

* Add CustomConvertor Test

* Add Compat tests

* Fix test failures

* Add telemetry

* Minor fix

* address comments

Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>

* Bump up build version to consume latest features

* Fix regressions (#306)

* Bump up build version to consume latest bug fixes

Co-authored-by: sunil-indoria <sindoria@microsoft.com>
Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>
Co-authored-by: Preben Huybrechts <hpreben@gmail.com>
Co-authored-by: jyotimisramsft <79437784+jyotimisramsft@users.noreply.github.com>
Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>
pakunapamsft added a commit to microsoft/service-fabric-services-and-actors-dotnet that referenced this issue Mar 3, 2022
* LocalStoreConfigFix (#288)

* Remove myget feed

* Bumpup build version to consume latest bug fixes

* Fix missing dependency upgrade

* Check in runtimeconfig.dev.json exists (#289)

.NET 6 doesn't create runtimeconfig.dev.json by default. See:  dotnet/sdk#16818

* Bumpup build version to consume latest bug fixes

* Modifying actor enumeration for KVS (#295)

* Modifying actor enumeration for KVS

* Addressing comments

* Minor change to prevent additional loop during actor enumeration

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Bump up build version to consume latest bug fixes

* Load KeyValueStore settings from settings.xml (#292)

* Bumpup build version to consume latest fixes

* Enumerate Reminders (#298)

* Add GetRemindersAsync state provider api

* Add GetReminders support in actor manager

* Buf fixes

* Bug fixes

* Build fix

* Enumerator dispose in KVS getactorsasync

* Bumpup build version to consume latest fixes

* Add GetRemindersAsync implementation in KVStoRCMigrationActorStateProvider

* Add Telemetry

* Add Migration Settings

* Refactor MigrationWorker framework

* Fix Issues

* Fix Issues

* V2 PagedResult fix (#300)

* V2 PagedResult fix

* Create ReminderPagedResult to serve GetRemindersAsync call

* Bump up build version to consume latest bug fixes

* Refactor migration orchestrator

* Remoting exception serialization  (#296)

* Add Service convertors

* Add client convertors

* Fix build issues

* Add actor convertors

* Add FabricExceptionConvertor test

* Add test cases

* Add CustomConvertor Test

* Add Compat tests

* Fix test failures

* Add telemetry

* Minor fix

* address comments

Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>

* Bump up build version to consume latest features

* temp commit

* Refactor

* Fix regressions (#306)

* Bump up build version to consume latest bug fixes

* Loosely couple migration

* Return from RunAsync

* Remove debug related code

* Temp commit

* Minor fixes

* Add request forwarder

* build fixes

* Implement request forward checking

* Fix actor events

* Add comments

* Add comments

* Enable way to add Migration listener by custom actor service

* Address comments

Co-authored-by: sunil-indoria <sindoria@microsoft.com>
Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>
Co-authored-by: Preben Huybrechts <hpreben@gmail.com>
Co-authored-by: jyotimisramsft <79437784+jyotimisramsft@users.noreply.github.com>
Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>
pakunapamsft added a commit to microsoft/service-fabric-services-and-actors-dotnet that referenced this issue Apr 1, 2022
* Migration state provider (#286)

* Adding migration state provider

* Adding reference to migration attribute

* Minor change

* Adding changes requested during PR

* Adding warning logs while creating default state provider

* Updating dictionaries used for saving KVS data in RC

* Minor change in warning message

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Task 10222517: Milestone1: Enable KVS service to be migration ready (#287)

* Add ActorMigrationAttribute

* Added GrpcCommunicationListner

* Initial changes to move to separate Actors.Migration library

* add migrationwebendpoint generation to fabactutil

* move to separate Actors.Migration library Part 2

* Add Actors.Migration Nuspec

* Task 10222604: Check if Tombstonecleanup is disabled

* Remove unnecessary package references

* Implement Kestrel listner for ASP.NET core WebApi

* Remove Grpc

* Added Models

* Clean up Debug code

* Addressed review comments. Removed depndency on Microsoft.ServiceFabric.AspNetCore. Ported Kestrel communication listener code from Microsoft.ServiceFabric.AspNetCore library

* moved dotnetframework to net461 for actors migration library

* Adding migration worker framework and moving relevant changes to migration library (#290)

* Migration worker framework

* Moved relevant migration code to migration library

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Updating Migration Worker Framework  (#293)

* Migration worker framework

* Moved relevant migration code to migration library

* Read stream response from KVS API

* Fixing formatting errors

* Updating kvs enumerate API

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Fix Http endpoint resolve and request logic (#301)

* Fix Http endpoint resolve and request logic

* Fix http uri

* Fix api paths (#302)

* Rc migration web apis (#299)

* Add RC migration Web APIs
Reject all actor write calls when RejectWrites is set to true

* Add RC migration Web APIs
Reject all actor write calls when RejectWrites is set to true

* Resolve build break after merge

* Add ThrowIfMigrationInProgress();

* Added GetRejectWriteSate

* Addressed review comments

* Changes for making calls from rc to kvs (#303)

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Kvs rc config validation (#305)

* Load KeyValueStore settings from settings.xml (#292)

* Task 13072430: KVS - RC configuration validation

Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>

* Rebase to develop (#308)

* LocalStoreConfigFix (#288)

* Remove myget feed

* Bumpup build version to consume latest bug fixes

* Fix missing dependency upgrade

* Check in runtimeconfig.dev.json exists (#289)

.NET 6 doesn't create runtimeconfig.dev.json by default. See:  dotnet/sdk#16818

* Bumpup build version to consume latest bug fixes

* Modifying actor enumeration for KVS (#295)

* Modifying actor enumeration for KVS

* Addressing comments

* Minor change to prevent additional loop during actor enumeration

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Bump up build version to consume latest bug fixes

* Load KeyValueStore settings from settings.xml (#292)

* Bumpup build version to consume latest fixes

* Enumerate Reminders (#298)

* Add GetRemindersAsync state provider api

* Add GetReminders support in actor manager

* Buf fixes

* Bug fixes

* Build fix

* Enumerator dispose in KVS getactorsasync

* Bumpup build version to consume latest fixes

* V2 PagedResult fix (#300)

* V2 PagedResult fix

* Create ReminderPagedResult to serve GetRemindersAsync call

* Bump up build version to consume latest bug fixes

* Remoting exception serialization  (#296)

* Add Service convertors

* Add client convertors

* Fix build issues

* Add actor convertors

* Add FabricExceptionConvertor test

* Add test cases

* Add CustomConvertor Test

* Add Compat tests

* Fix test failures

* Add telemetry

* Minor fix

* address comments

Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>

* Bump up build version to consume latest features

* Fix regressions (#306)

* Bump up build version to consume latest bug fixes

Co-authored-by: sunil-indoria <sindoria@microsoft.com>
Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>
Co-authored-by: Preben Huybrechts <hpreben@gmail.com>
Co-authored-by: jyotimisramsft <79437784+jyotimisramsft@users.noreply.github.com>
Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* KVS to RC Actor state migration - Basic implementation (#304)

* LocalStoreConfigFix (#288)

* Remove myget feed

* Bumpup build version to consume latest bug fixes

* Fix missing dependency upgrade

* Check in runtimeconfig.dev.json exists (#289)

.NET 6 doesn't create runtimeconfig.dev.json by default. See:  dotnet/sdk#16818

* Bumpup build version to consume latest bug fixes

* Modifying actor enumeration for KVS (#295)

* Modifying actor enumeration for KVS

* Addressing comments

* Minor change to prevent additional loop during actor enumeration

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Bump up build version to consume latest bug fixes

* Load KeyValueStore settings from settings.xml (#292)

* Bumpup build version to consume latest fixes

* Enumerate Reminders (#298)

* Add GetRemindersAsync state provider api

* Add GetReminders support in actor manager

* Buf fixes

* Bug fixes

* Build fix

* Enumerator dispose in KVS getactorsasync

* Bumpup build version to consume latest fixes

* Add GetRemindersAsync implementation in KVStoRCMigrationActorStateProvider

* Add Telemetry

* Add Migration Settings

* Refactor MigrationWorker framework

* Fix Issues

* Fix Issues

* V2 PagedResult fix (#300)

* V2 PagedResult fix

* Create ReminderPagedResult to serve GetRemindersAsync call

* Bump up build version to consume latest bug fixes

* Refactor migration orchestrator

* Remoting exception serialization  (#296)

* Add Service convertors

* Add client convertors

* Fix build issues

* Add actor convertors

* Add FabricExceptionConvertor test

* Add test cases

* Add CustomConvertor Test

* Add Compat tests

* Fix test failures

* Add telemetry

* Minor fix

* address comments

Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>

* Bump up build version to consume latest features

* temp commit

* Refactor

* Fix regressions (#306)

* Bump up build version to consume latest bug fixes

* Loosely couple migration

* Return from RunAsync

* Remove debug related code

* Temp commit

* Minor fixes

* Add request forwarder

* build fixes

* Implement request forward checking

* Fix actor events

* Add comments

* Add comments

* Enable way to add Migration listener by custom actor service

* Address comments

Co-authored-by: sunil-indoria <sindoria@microsoft.com>
Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>
Co-authored-by: Preben Huybrechts <hpreben@gmail.com>
Co-authored-by: jyotimisramsft <79437784+jyotimisramsft@users.noreply.github.com>
Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Manual Migration (#311)

* Pakunapa/kvs rc migration/exceptionhandling (#312)

* Exception Handling

* use constants

* Fix NPE

* Fix NPE on secondaries (#316)

* Fix NPE on secondaries

* Minor Fix

* Bug fixes identified during E2E migration test (#318)

* Changes for making calls from rc to kvs

* KvsControllerBugFix

* Enumerate kvs data bug fix

Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>

* Fix issues and enable winfab tests (#317)

Co-authored-by: jyotimisramsft <79437784+jyotimisramsft@users.noreply.github.com>
Co-authored-by: Jyoti Misra <jyotimisra@microsoft.com>
Co-authored-by: Roshan Routray <11490554+roshanroutray@users.noreply.github.com>
Co-authored-by: PavanKunapareddy <pavankumar.kunapareddy@gmail.com>
Co-authored-by: sunil-indoria <sindoria@microsoft.com>
Co-authored-by: Preben Huybrechts <hpreben@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants