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

Fix winget after a call to winget settings export #2767

Merged
merged 8 commits into from
Dec 15, 2022

Conversation

msftrubengu
Copy link
Contributor

@msftrubengu msftrubengu commented Dec 14, 2022

Fix bug introduced by #2719

Using Runtime::GetPathTo uses PathDetails which creates a directory when PathDetails.Create is true (which is the default). When winget settings export is called, that makes the code create a directory named the same as the settings file. Now, next winget run will try to open the file but it will find out it is a path and give up on life.

Since Runtime::GetPathTo is more related to path, getting the user settings json file path is not the best option. Move logic to UserSettings.cpp. Also catch the exception when trying to parse the fail to not make winget fail but use the default settings is something happens.

Microsoft Reviewers: Open in CodeFlow

@msftrubengu msftrubengu requested a review from a team as a code owner December 14, 2022 05:41
@msftrubengu msftrubengu changed the title Fix Fix winget after a call to winget settings export Dec 14, 2022
Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

}
catch (...)
{
AICLI_LOG(Core, Error, << "Failed to read settings.json file.. Reason unknown.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
AICLI_LOG(Core, Error, << "Failed to read settings.json file.. Reason unknown.");
AICLI_LOG(Core, Error, << "Failed to read settings.json file. Reason unknown.");

return ParseSettingsContent(settingsContentStr, setting.Name, warnings);
}
}
catch (const std::exception& e)
Copy link
Member

Choose a reason for hiding this comment

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

This exposes the scenario where both the normal and backup settings fail, which can result in no output to the user about settings load failure? I think it just means an else for when the backup settings fails to load (~line 520 now) where we put in a different warning about not being able to load any settings file.

@@ -518,6 +518,10 @@ namespace AppInstaller::Settings
m_type = UserSettingsType::Backup;
settingsRoot = settingsBackupJson.value();
}
else
{
AICLI_LOG(Core, Warning, << "Failed loading settings files.");
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more along the lines of the user visible portion (without opening the log file). So a new:

m_warnings.emplace_back(StringResource::String::SettingsWarningWhoopsSettingsDidntLoad);

That would then at least warn the user "Hey, we couldn't load any settings, so you are getting the default behavior".

@@ -933,7 +934,8 @@ namespace AppInstaller::CLI
{
try
{
if (!Settings::User().GetWarnings().empty())
if (!Settings::User().GetWarnings().empty() &&
!WI_IsFlagSet(command->GetOutputFlags(), CommandOutputFlags::IgnoreSettingsWarnings))
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, I would think this would be better done using the output channels mechanism, but this is acceptable.

The nice thing about that mechanism being built out is that any error/exception could be output in a JSON format as well. But we have only so much time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants