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

add SpecialFolder.UserProfile fallback #6072

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kasperk81
Copy link

Description

when HOME is not set in container, nuget commands fail while other sdk commands work due to SpecialFolder.UserProfile fallback https://github.com/dotnet/sdk/blob/c3a8f72c3a5491c693ff8e49e7406136a12c3040/src/Common/CliFolderPathCalculatorCore.cs#L38-L38.

nuget already uses UserProfile with full framework, this pr adds the same fallback for coreclr.

  The "WarnForInvalidProjectsTask" task failed unexpectedly.
  System.InvalidOperationException: Required environment variable 'HOME' is not set. Try setting 'DOTNET_CLI_HOME' or 'HOME' and running the operation
     at NuGet.Common.NuGetEnvironment.GetValueOrThrowMissingEnvVarsDotnet(Func`1 getValue, String home, String dotnetHome)
     at NuGet.Common.NuGetEnvironment.GetHome()
     at NuGet.Common.NuGetEnvironment.<>c.<.cctor>b__20_0()
     at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
     at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
     at System.Lazy`1.CreateValue()
     at NuGet.Common.NuGetEnvironment.GetFolderPath(SpecialFolder folder)
     at NuGet.Common.NuGetEnvironment.GetFolderPath(NuGetFolderPath folder)
     at NuGet.Common.PathUtility.CheckIfFileSystemIsCaseInsensitive()
     at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
     at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
     at System.Lazy`1.CreateValue()
     at NuGet.Common.PathUtility.get_IsFileSystemCaseInsensitive()
     at NuGet.Common.PathUtility.GetStringComparerBasedOnOS()
     at NuGet.Build.Tasks.WarnForInvalidProjectsTask.Execute()
     at Microsoft.Build.BackEnd.TaskExecutionHost.Execute()
     at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(TaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask)

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests (idk, how to test it in ci)
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@kasperk81 kasperk81 requested a review from a team as a code owner October 1, 2024 22:06
@microsoft-github-policy-service microsoft-github-policy-service bot added the Community PRs created by someone not in the NuGet team label Oct 1, 2024
@@ -313,7 +314,8 @@ private static string GetHomeWindows()
}
else
{
return Environment.GetEnvironmentVariable("HOMEDRIVE") + Environment.GetEnvironmentVariable("HOMEPATH");
return Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) ??
Copy link
Contributor

@kartheekp-ms kartheekp-ms Oct 2, 2024

Choose a reason for hiding this comment

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

Lines 310 to 314 contain the logic to return the UserProfile environment variable if it is set. Therefore, this change is not necessary. In summary, the changes made to this method can be reverted.

@@ -290,7 +290,8 @@ private static string GetHome()
}
else
{
return GetValueOrThrowMissingEnvVarsDotnet(() => GetDotNetHome() ?? Environment.GetEnvironmentVariable(Home), Home, DotNetHome);
return GetValueOrThrowMissingEnvVarsDotnet(() =>
GetDotNetHome() ?? Environment.GetEnvironmentVariable(Home) ?? Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), Home, DotNetHome);
Copy link
Member

Choose a reason for hiding this comment

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

SpecialFolder didn't exist in .NET Core 1.0, which is why I guess NuGet's custom home finding method was added. But since the API exists now, I think we should delete it and only use the .NET API.

This might be a breaking change, which is why our PR template asks for a linked issue in NuGet/Home. Those issues get linked in our release notes, and raise awareness of such changes.

Frankly I'm astonished that containers exist where the HOME environment variable isn't set. Surely NuGet isn't the only app that doesn't expect this. Why doesn't Docker make it a pre-defined variable, rather than trying to get all other tools to change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants