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.

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

let user know working directory will be used as content root path by default #82445

Merged
merged 4 commits into from
Apr 26, 2023
Merged

let user know working directory will be used as content root path by default #82445

merged 4 commits into from
Apr 26, 2023

Conversation

pedrobsaila
Copy link
Contributor

Fixes #78789

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 21, 2023
kasperk81
kasperk81 reviewed Feb 21, 2023
View reviewed changes
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.cs Outdated
@@ -79,6 +80,11 @@ private void OnApplicationStarted()
Logger.LogInformation("Application started. Press Ctrl+C to shut down.");
Logger.LogInformation("Hosting environment: {EnvName}", Environment.EnvironmentName);
Logger.LogInformation("Content root path: {ContentRoot}", Environment.ContentRootPath);

if (Path.GetFullPath(Environment.ContentRootPath).Equals(Path.GetFullPath("."), StringComparison.InvariantCultureIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

Path.GetFullPath(".")

Envrionment.CurrentDirectory?

InvariantCultureIgnoreCase

wrong on unix?

Copy link
Contributor Author

@pedrobsaila pedrobsaila Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Envrionment.CurrentDirectory ?

Envrionment.CurrentDirectory when !DisableDefaults, and AppContext.BaseDirectory elsewhere

wrong on unix?

This project uses OrdinalIgnoreCase for string comparison so I'm using Ordinal to be unix compliant

@build-analysis build-analysis bot mentioned this pull request Feb 21, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Feb 22, 2023
Closed
jasper-d
jasper-d reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@jasper-d jasper-d left a comment

Choose a reason for hiding this comment

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

Could this be logged as info instead?

Some apps do not care about content root and would now need to explicitly configure it anyways or add extra logging config just to avoid that this warning pollutes observability systems.

@ghost
Copy link

ghost commented Mar 1, 2023

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #78789

Author: pedrobsaila
Assignees: -
Labels:

area-Extensions-Hosting, community-contribution
Milestone: -

@steveharter steveharter self-requested a review March 1, 2023 18:24
steveharter
steveharter reviewed Mar 13, 2023
View reviewed changes
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.cs
string contentRootFullPath = Path.GetFullPath(Environment.ContentRootPath);

if (contentRootFullPath.Equals(System.Environment.CurrentDirectory, StringComparison.Ordinal)
|| contentRootFullPath.Equals(AppContext.BaseDirectory, StringComparison.Ordinal))
Copy link
Member

Choose a reason for hiding this comment

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

When are these different?

Copy link
Contributor Author

@pedrobsaila pedrobsaila Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • when DisableDefaults == false, it is System.Environment.CurrentDirectory :

    • code :

runtime/src/libraries/Microsoft.Extensions.Hosting/src/HostingHostBuilderExtensions.cs

Lines 216 to 227 in d3f7d59

// In my testing, both Environment.CurrentDirectory and Environment.GetFolderPath(Environment.SpecialFolder.System) return the path without
// any trailing directory separator characters. I'm not even sure the casing can ever be different from these APIs, but I think it makes sense to
// ignore case for Windows path comparisons given the file system is usually (always?) going to be case insensitive for the system path.
string cwd = Environment.CurrentDirectory;
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || !string.Equals(cwd, Environment.GetFolderPath(Environment.SpecialFolder.System), StringComparison.OrdinalIgnoreCase))
{
hostConfigBuilder.AddInMemoryCollection(new[]
{
new KeyValuePair<string, string?>(HostDefaults.ContentRootKey, cwd),
});
}
}

  • test :

https://github.com/dotnet/runtime/blob/d3f7d59e8dc44e5ce22c8ec9c011a1b72d7e9e92/src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/HostApplicationBuilderTests.cs#LL519C9-L524C10

  • else it is AppContext.BaseDirectory :

    • code :

runtime/src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

Lines 230 to 234 in d3f7d59

var hostingEnvironment = new HostingEnvironment()
{
EnvironmentName = hostConfiguration[HostDefaults.EnvironmentKey] ?? Environments.Production,
ContentRootPath = ResolveContentRootPath(hostConfiguration[HostDefaults.ContentRootKey], AppContext.BaseDirectory),
};

  • test :

https://github.com/dotnet/runtime/blob/d3f7d59e8dc44e5ce22c8ec9c011a1b72d7e9e92/src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/HostApplicationBuilderTests.cs#LL251C9-L283C10

Copy link
Contributor Author

@pedrobsaila pedrobsaila Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if there's a case when they have different values, but the two seems different by looking at their code

@dotnet dotnet deleted a comment from azure-pipelines bot Apr 19, 2023
@dotnet dotnet deleted a comment from azure-pipelines bot Apr 19, 2023
@steveharter
Copy link
Member

/azp run runtime-dev-innerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveharter
Copy link
Member

There are a few CI errors; re-running

@steveharter
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveharter steveharter merged commit c958573 into dotnet:main Apr 26, 2023
@pedrobsaila pedrobsaila deleted the 78789 branch April 27, 2023 07:26
@JamesNK JamesNK mentioned this pull request May 5, 2023
Closed
steveharter added a commit to steveharter/runtime that referenced this pull request May 10, 2023
@steveharter
Revert "let user know working directory will be used as content root …
ae2de5b 
…path by default (dotnet#82445)"

This reverts commit c958573.
steveharter added a commit that referenced this pull request May 10, 2023
@steveharter
Revert "let user know working directory will be used as content root …
d80ba2f 
…path by default (#82445)" (#86057)

This reverts commit c958573.
@steveharter
Copy link
Member

@pedrobsaila there were concerns with this expressed in #85809. Please see that issue. Thanks.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jasper-d jasper-d jasper-d left review comments

@kasperk81 kasperk81 kasperk81 left review comments

@steveharter steveharter steveharter approved these changes

Assignees
No one assigned
Labels
area-Extensions-Hosting community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Explictily log working directory and let user know it will be used as content root path by default.
4 participants
@pedrobsaila @steveharter @jasper-d @kasperk81