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

Provide a way to override the global configuration file path #931

Closed
JeremyKuhne opened this issue Sep 4, 2018 · 21 comments · Fixed by #56748
Closed

Provide a way to override the global configuration file path #931

JeremyKuhne opened this issue Sep 4, 2018 · 21 comments · Fixed by #56748
Assignees
Labels
area-System.Configuration good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@JeremyKuhne
Copy link
Member

Need to follow through how the default value gets set on .NET Framework and see if we can match. Notably, if there is a way to configure this via environment variables we should also allow doing this and update System.Configuration to pull from AppDomainSetup.ConfigurationFile.

At worst we should move the logic down from System.Configuration for generating this name.

Note that this value is settable. I believe that there is no way to get System.Configuration to reset after it does it's first load on the .NET Framework, but that should also be investigated.

@JeremyKuhne
Copy link
Member Author

Note that I don't expect this to "fix" dotnet test. I've opened an issue on vstest to try and get the ability to set up config files when testing.

@danmoseley
Copy link
Member

Came up with another major would-be porter.

@maryamariyan maryamariyan removed their assignment Mar 18, 2019
@danmoseley
Copy link
Member

@JeremyKuhne any new info on this one ? Wondering whether we must have it for 3.0.

@JeremyKuhne
Copy link
Member Author

Wondering whether we must have it for 3.0.

Must? I don't think so, but this is painful for porting when using System.Configuration.

One fundamental problem is that we don't expose ConfigurationFile on AppDomainSetup anymore. On desktop that is how you found the current app.config. Test frameworks could customize the path when creating a new AppDomain to run tests on.

While we don't have multiple AppDomains any more we have one as our primary context. We could re-add ConfigurationFile and pull the value from the AppContext data store as we're doing for APP_CONTEXT_BASE_DIRECTORY. So adding:

public static partial class AppContext
{
    // Populated with "APP_CONTEXT_CONFIGURATION_FILE" much like "APP_CONTEXT_BASE_DIRECTORY"
    public static ConfigurationFile { get; }
}

public static partial class AppDomainSetup
{
    // This API exists on NetFX
    public static ConfigurationFile => AppContext.ConfigurationFile;
}

If this was set before System.Configuration grabs the value for the first time that would help some scenarios. That could be done via AppContext.SetData() or passed in through the cli's corehost (which would require additional work). Potentially this could alternatively be pushed via @vitek-karas startup hook?

Putting the hooks in our side would be relatively easy as you could follow the pattern used by APP_CONTEXT_BASE_DIRECTORY.

@vitek-karas
Copy link
Member

I'm not sure what is the trigger to System.Configuration to grab the value for the first time, but I assume it's nothing in the CoreLib itself. In that case startup hook should be able to set the value in AppContext before this happens (via AppContext.SetData) since it runs before Main. So unless the startup hook itself somehow triggers System.Configuration it should work just fine.

CoreCLR's native host can already do this - in fact it will already work today via .runtimeconfig.json. It is possible to specify any property name/value pair which will be copied verbatim to the AppContext.GetData dictionary on runtime init. We're working on enabling custom native hosts to do the same programmatically through hosting APIs (but I guess that is not very interesting for the test scenarios).

@JeremyKuhne
Copy link
Member Author

I'm not sure what is the trigger to System.Configuration to grab the value for the first time, but I assume it's nothing in the CoreLib itself.

It is just the static constructor. As we don't use it directly, user code should be able to hit it.

It is possible to specify any property name/value pair

What is the syntax for that?

@JeremyKuhne JeremyKuhne self-assigned this Apr 15, 2019
@vitek-karas
Copy link
Member

In the app's .runtimeconfig.json

{
  "runtimeOptions": {
    "tfm": "netcoreapp3.0",
    "configProperties": {
        "APP_CONTEXT_CONFIGURATION_FILE": "myfile.config"
    },
    "framework": {
      "name": "Microsoft.NETCore.App",
      "version": "3.0.0"
    }
  }
}

And then from managed code AppContext.GetData("APP_CONTEXT_CONFIGURATION_FILE").

@jkotas
Copy link
Member

jkotas commented Apr 15, 2019

this is painful for porting when using System.Configuration.

Do we have a customer report about what people end up doing to make this work today? It would be useful to validate that this proposal will make it significantly better.

We do not have end-to-end support for the XML config files in the .NET Core tooling. Unless we fix that, the XML config file experience on .NET Core is always going somewhat worse compared to .NET Framework.

This is very similar to https://github.com/dotnet/corefx/issues/36433. Both are about components that just need piece of data, and this piece of data happened to live with AppDomainSetup in .NET Framework.

@JeremyKuhne
Copy link
Member Author

Do we have a customer report about what people end up doing to make this work today?

They are having to rewrite their code or copy over the shared test app.config on the box.

Unless we fix that, the XML config file experience on .NET Core is always going somewhat worse compared to .NET Framework.

Yeah, I don't really expect to make this painless, there just isn't a way to do this now. Idea is MSTest can just push the app.config for the actual tested assembly using this mechanism- or as a hack users can poke it themselves if they get to it early enough.

I don't imagine there is a lot of code out there that needs to read the config path out of AppDomainSetup, but it wouldn't hurt to have it for any other cases that do.

@JeremyKuhne
Copy link
Member Author

The planned workaround here is to read from `AppDomain.CurrentDomain.GetData("APP_CONTEXT_CONFIGURATION_FILE") and use that if it exists.

Here is where the change needs to be made:

https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs#L63

Overiding can be done via https://github.com/dotnet/corefx/issues/32095#issuecomment-483425717 or making sure you get in and set the value on AppDomain before ConfigurationManager is first used.

dotnet/corefx#36897 tracks creating an API to move this logic up to AppDomainSetup.

@bricelam
Copy link
Contributor

bricelam commented May 28, 2019

cc @divega

This hook would help the EF6 tests. For now, we work around it with a bit of MSBuild magic:

<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.0'">
  <None Update="App.config">
    <Link>xunit.console.dll.config</Link>
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
  </None>
</ItemGroup>

Note, xunit.console.dll is the entry assembly when running our tests.

@danmoseley
Copy link
Member

Moving to Future as this is not required to ship 3.0.

@jkotas
Copy link
Member

jkotas commented Sep 27, 2019

We are done with porting APIs from .NET Framework.

@jkotas jkotas closed this as completed Sep 27, 2019
@bricelam
Copy link
Contributor

bricelam commented Sep 27, 2019

@jkotas Is this fixed? This issue grew into something larger than the title suggests. To me, this issue is about providing a hook (e.g. AppDomain.CurrentDomain.SetData("APP_CONTEXT_CONFIGURATION_FILE", testConfigFile)) for things like test runners and tools to use something besides the entry assembly name when reading config.

@jkotas jkotas changed the title Investigate providing a rational AppDomainSetup.ConfigurationFile value Provide a way to overriding configuration file path Sep 27, 2019
@jkotas
Copy link
Member

jkotas commented Sep 27, 2019

Ok, I have updated the title to match.

@jkotas jkotas reopened this Sep 27, 2019
@jkotas jkotas changed the title Provide a way to overriding configuration file path Provide a way to override the global configuration file path Sep 27, 2019
@maryamariyan maryamariyan transferred this issue from dotnet/corefx Dec 16, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Configuration untriaged New issue has not been triaged by the area owner labels Dec 16, 2019
@maryamariyan maryamariyan added good first issue Issue should be easy to implement, good for first-time contributors tenet-compatibility Incompatibility with previous versions or .NET Framework help wanted [up-for-grabs] Good issue for external contributors labels Dec 16, 2019
@ericsampson
Copy link

Is there any interest in addressing this? We keep hitting it when porting from Framework, and it's kinda annoying.

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 16, 2021
@joperezr
Copy link
Member

joperezr commented May 6, 2021

@ericsampson We have an API which is already approved here #29271 and it is up-for-grabs. When adding the implementation for it, we only need to make sure it is based on an AppContext switch as proposed in that issue so that it can be used to override the global configuration file. After that is done, we just need to update System.Configuration so that it uses this AppContext value as @JeremyKuhne mentioned here #931 (comment)

Would you be interested in contributing the fix and API for this?

@joperezr joperezr modified the milestones: Future, 6.0.0 May 6, 2021
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this issue Jul 1, 2021
@jkotas
Copy link
Member

jkotas commented Jul 30, 2021

AppDomain.CurrentDomain.SetData("APP_CONTEXT_CONFIGURATION_FILE", testConfigFile))

.NET Framework actually used APP_CONFIG_FILE for this setting. This should be fixed by using the same.

@krwq krwq self-assigned this Jul 30, 2021
@danmoseley
Copy link
Member

danmoseley commented Jul 30, 2021

Will this indeed fix dotnet/project-system#7448 as well? It's not clear to me.

@eerhardt
Copy link
Member

eerhardt commented Jul 30, 2021

No, I don't believe this will fix dotnet/project-system#7448.

This issue is about allowing an app to specify a custom file location for their config file. dotnet/project-system#7448 is about the config system failing to initialize when a system.diagnostics section is present.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 30, 2021

No, I don't believe this will fix dotnet/project-system#7448.

Yes, the original issue is not related, but the issue mentioned in the comment is related to this and which made me think this can fix dotnet/project-system#7448. Now i think dotnet/project-system#7448 is either non-runtime issue or might be related to #32307

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Aug 2, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 5, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Configuration good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
No open projects