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

API proposal: Add AppContext.ApplicationConfig #29271

Closed
JeremyKuhne opened this issue Apr 15, 2019 · 18 comments
Closed

API proposal: Add AppContext.ApplicationConfig #29271

JeremyKuhne opened this issue Apr 15, 2019 · 18 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Configuration help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@JeremyKuhne
Copy link
Member

While app.config isn't the way we configure Core, plenty of ported libraries still use System.Configuration. We don't expose ConfigurationFile on AppDomainSetup any more and we hack it's path directly in System.Configuration.

A common pattern, particularly for testing frameworks, is to fire up a new AppDomain with a custom config file. As we only have a single AppDomain on Core this isn't possible and is creating some serious pain in porting larger more complicated projects. We can provide the customization at the hosting level by exposing a property like APP_CONTEXT_CONFIGURATION_FILE much like we do APP_CONTEXT_BASE_DIRECTORY.

public partial class AppDomainSetup
{
    // This API exists on NetFX
    // Populated with APP_CONTEXT_CONFIGURATION_FILE by default
    public string ConfigurationFile { get; set; }
}

See #931 and linked issues for more.

We could also add another field to AppContext to see what this setting is, but it isn't strictly necessary. For APP_CONTEXT_BASE_DIRECTORY we expose it from AppContext.BaseDirectory. To follow suit we would add (and populate the default for AppDomainSetup above from this):

public static partial class AppContext
{
    // Populated with "APP_CONTEXT_CONFIGURATION_FILE" much like "APP_CONTEXT_BASE_DIRECTORY"
    public static string ConfigurationFile { get; }
}
@JeremyKuhne JeremyKuhne self-assigned this Apr 15, 2019
@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

If we are doing this just for compatibility with .NET Framework, we should just add the exact API that exists for this in .NET Framework and nothing more: https://docs.microsoft.com/en-us/dotnet/api/system.appdomainsetup.configurationfile?view=netframework-4.7.2

public static partial class AppDomainSetup
{
    public string ConfigurationFile { get; set; }
}

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

We can provide the customization at the hosting level

I do not think that this makes sense to propagate this legacy stuff into the host. The host is complicated enough as is.

@JeremyKuhne
Copy link
Member Author

I do not think that this makes sense to propagate this legacy stuff into the host. The host is complicated enough as is.

We already have the ability to pass in arbitrary key/value pairs through the host into AppContext.GetData(), why not use it? It isn't like we're adding anything complicated here and it makes it possible to truly get in front of the static constructor for ConfigurationManager.

I can live with not adding another property on AppContext if we really don't want it, it just seemed to make sense to match our existing hook. As long as we pull it from a host property pair if it exists I'm perfectly happy. We can hold off on anything more complicated until we have stronger scenario justification.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

We already have the ability to pass in arbitrary key/value pairs

The AppDomainSetup.ConfigurationFile property is a full path. The arbitrary key/value pairs do not work well for full paths - they get invalidated when the app moves. Which part of the system would compute the full path for AppDomainSetup.ConfigurationFile?

it makes it possible to truly get in front of the static constructor for ConfigurationManager

I do not understand the problem with getting in front of the static constructor for ConfigurationManager. This constructor will run only once something uses the ConfigurationManager. If the app wants to configure the path to the configuration file, it can do it in the Main method before using the configuration manager. It is similar how one configures other global defaults, e.g. CultureInfo.DefaultThreadCurrentCulture.

My suggestion would be to introduce a new static string ConfigurationManager.ConfigurationFile { get; set; } property. The default value of this property would be path to app binary with suffix changed to .xml. If the default does not work for the app, the app can change it to some other value before the configuration system gets initialized. The advantage of this is that the solution is fully contained within the System.Configuration and not spread through the system.

@JeremyKuhne
Copy link
Member Author

The arbitrary key/value pairs do not work well for full paths - they get invalidated when the app moves.

You're responsible for giving a full path if you use this method. Much as you would be if you were passing this in on desktop or setting the property directly before spinning up a new appdomain.

If the app wants to configure the path to the configuration file, it can do it in the Main method before using the configuration manager.

It isn't always possible due to external dependencies, etc. In addition, test harnesses can't make a universal fix without this. They need to point to the "normal" app.config before user code runs.

My suggestion would be to introduce a new static string ConfigurationManager.ConfigurationFile { get; set; } property

I've considered that but I don't want to do it for a couple of reasons.

  1. Rearchitecting to allow setting this is risky. Configuration code is convoluted.
  2. Still won't allow test harnesses to fix universally. This forces the problem on the users.
  3. Having multiple hacks for the configuration file seems risky. Having one source of truth matches historical behavior- you didn't have multiple config file strings per domain.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

test harnesses can't make a universal fix without this

So how does test harnesses make a universal fix with the API proposed at the top, without rearchitecting to allow setting this?

@JeremyKuhne
Copy link
Member Author

So how does test harnesses make a universal fix with the API proposed at the top, without rearchitecting to allow setting this?

They push the config file path through when the host is launched, or possibly hook into the new pre-main extensibility.

https://github.com/dotnet/corefx/issues/32095#issuecomment-483425717
https://github.com/dotnet/core-setup/blob/0cc40ba37ae41be6f66128278df898c4f6ea0a85/Documentation/design-docs/host-startup-hook.md

Passing the value through to the CLR host seems like the most dependable way and that would be what I would recommend when possible.

I'm particularly skittish about this as I hit a similar problem with PowerShell not setting the long path flags until after they started the host and started running code. They just programmatically set the flag, despite my objections. Something else got in front and floored a bunch of internal tools recently. It took coming to me for them to figure out what was happening and what they could do to mitigate the problem. Given that any of the options here are often insanely difficult for people to diagnose when they go wrong I'd much rather go with the hardest to accidentally break option. :)

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

They push the config file path through when the host is launched, or possibly hook into the new pre-main extensibility.

Ok, we do not really need any new API to make this work. All that needs to happen for this is that System.Configuration calls AppContext.GetData to read what was set in the config file.

@JeremyKuhne
Copy link
Member Author

Ok, we do not really need any new API to make this work.

For the pass in key-value pair, sure. You can't programmatically set via the host startup hook this way however. I've split out the "bring back" and "new, but not strictly needed" APIs above.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

You can't programmatically set via the host startup hook this way however

You can: AppContext.SetData("APP_CONTEXT_CONFIGURATION_FILE", "Whatever") in the startup hook will work just fine.

@JeremyKuhne
Copy link
Member Author

You can: AppContext.SetData("APP_CONTEXT_CONFIGURATION_FILE", "Whatever") in the startup hook will work just fine.

We don't currently expose it: https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L102

@jkotas
Copy link
Member

jkotas commented Apr 17, 2019

Hmmm, I have not realized that this one of those APIs that are public in CoreLib, but not exposed in refs.

AppDomain.CurrentDomain.SetData("APP_CONTEXT_CONFIGURATION_FILE", "Whatever") is exposed and it does the same thing.

@JeremyKuhne
Copy link
Member Author

is exposed and it does the same thing.

Missed that myself. :) I'll implement based on that to start/unblock, but I'd still like to see it come from the property.

@jkotas
Copy link
Member

jkotas commented Apr 17, 2019

It is fine to have API for configuration values like these, but the APIs should live together with the component that they are configuring. It does not make sense to have a configuration for every component out there in CoreLib. The random grab-bag of things on AppDomainSetup in desktop was not a good design.

@shvez
Copy link

shvez commented Jul 2, 2019

Is there any progress on this?

For us, such a feature would be really really useful.
The only thing I do not get whether this new feature will work with applications loaded as a plugin?

our use case is: we have netcoreapp host application which tries to load as plugins different modules. Every of such modules has its own config file - assembly_name.dll.config.
We tried to find out a workaround but it does not work stable enough. To make it stable we add a dependency from System.Configuration.ConfigurationManager to host application, although it does not need it

@terrajobst
Copy link
Member

terrajobst commented Jul 9, 2019

Video

Conclusion

  • Exposing the API on AppDomainSetup is fine, but we shouldn't expose the property on AppContext
  • We may want to expose the backing store for .NET Core's runtimeconfig.json (and that should go on AppContext) but it would point to a different file, obviously.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@JeremyKuhne
Copy link
Member Author

@maryamariyan Fyi, this is System.Configuration specific.

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@JeremyKuhne JeremyKuhne removed their assignment Mar 25, 2020
@JeremyKuhne JeremyKuhne added the help wanted [up-for-grabs] Good issue for external contributors label Mar 25, 2020
@buyaa-n buyaa-n modified the milestones: Future, 6.0.0 May 6, 2021
@ghost ghost moved this from Future to Needs triage in Triage POD for Meta, Reflection, etc May 6, 2021
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label May 6, 2021
@buyaa-n buyaa-n moved this from Needs triage to v-Next in Triage POD for Meta, Reflection, etc May 6, 2021
@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label May 6, 2021
@krwq krwq modified the milestones: 6.0.0, 7.0.0, Future Jul 22, 2021
@krwq krwq moved this from 6.0 to Future in Triage POD for Meta, Reflection, etc Jul 22, 2021
@ghost ghost moved this from Future to Needs triage in Triage POD for Meta, Reflection, etc Jul 22, 2021
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jul 22, 2021
@krwq krwq removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jul 23, 2021
@krwq krwq moved this from Needs triage to Future in Triage POD for Meta, Reflection, etc Jul 23, 2021
@jkotas
Copy link
Member

jkotas commented Jul 30, 2021

I do not think it makes sense to add this API to support legacy configuration system since we have lived for multiple releases without it now.

@jkotas jkotas closed this as completed Jul 30, 2021
Triage POD for Meta, Reflection, etc automation moved this from Future to Done Jul 30, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Configuration help wanted [up-for-grabs] Good issue for external contributors
Projects
No open projects
Development

No branches or pull requests

8 participants