Skip to content

[905583]Fix to catch unauthorized exception when config file is created while setting sdkpath #71

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

Closed
wants to merge 4 commits into from
Closed

[905583]Fix to catch unauthorized exception when config file is created while setting sdkpath #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4dca19a
Add try/catch to handle an unauthorized permission exception while cr…
GouriKumari Jun 6, 2019
72a7605
Change the return value of saveconfigdoc to bool for validation durin…
GouriKumari Jun 6, 2019
07d101b
Fix formatting
GouriKumari Jun 6, 2019
1db3fa4
Remove redundant temp variable
GouriKumari Jun 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,12 @@ internal static void DefaultConsoleLogger (TraceLevel level, string message)
}
}

public static void SetPreferredAndroidSdkPath (string path, Action<TraceLevel, string> logger = null)
public static bool SetPreferredAndroidSdkPath (string path, Action<TraceLevel, string> logger = null)
Copy link
Contributor

@jonpryor jonpryor Jun 10, 2019

Choose a reason for hiding this comment

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

This is "public"-ish API, used in numerous places, and it might not be easy to keep all such places synchronized with each other. This should thus be treated as an ABI break and avoided if at all possible.

Additionally, I fail to see why it would be a "bad" thing for AndroidSdkInfo.SetPreferredAndroidSdkPath() to throw an exception when it can't save the specified value. Why can't the caller catch the exception?

Copy link
Author

@GouriKumari GouriKumari Jun 11, 2019

Choose a reason for hiding this comment

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

This issue is caused because user do not have the right permissions to create the $HOME/.config/xbuild/monodroid-config.xml and it is caused by 4dca19a#diff-c96432cf25d2723c79c03d426742f345L192. The issue is coming from xamarin-android-tools which is a submodule for installer. My fix is to handle this issue properly in android-tools, so that installer can handle it with an error slide and the user knows that installer do not have the right permissions to create $HOME/.config/xbuild/monodroid-config.xml. This is not hiding the error but a way to show user that there is a permission issue and installer cannot continue Xamarin.Android installation.

{
logger = logger ?? DefaultConsoleLogger;

var sdk = CreateSdk (logger);
sdk.SetPreferredAndroidSdkPath (path);
var sdk = CreateSdk(logger);
return sdk.SetPreferredAndroidSdkPath(path);
}
Copy link
Contributor

@rodrmoya rodrmoya Jun 7, 2019

Choose a reason for hiding this comment

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

here, no need for the temp var, just return sdk.SetPreferredAndroidSdkPath(path); should be enough


public static void SetPreferredJavaSdkPath (string path, Action<TraceLevel, string> logger = null)
Expand Down
2 changes: 1 addition & 1 deletion src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public virtual void Initialize (string androidSdkPath = null, string androidNdkP
protected abstract string GetJavaSdkPath ();
protected abstract string GetShortFormPath (string path);

public abstract void SetPreferredAndroidSdkPath (string path);
public abstract bool SetPreferredAndroidSdkPath (string path);
public abstract void SetPreferredJavaSdkPath (string path);
public abstract void SetPreferredAndroidNdkPath (string path);

Expand Down
32 changes: 21 additions & 11 deletions src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ protected override string GetShortFormPath (string path)
return path;
}

public override void SetPreferredAndroidSdkPath (string path)
public override bool SetPreferredAndroidSdkPath (string path)
{
path = NullIfEmpty (path);

Expand All @@ -145,8 +145,9 @@ public override void SetPreferredAndroidSdkPath (string path)
doc.Root.Add (androidEl);
}

androidEl.SetAttributeValue ("path", path);
SaveConfig (doc);
androidEl.SetAttributeValue("path", path);
bool setConfig = SaveConfig(doc, Logger);
return setConfig;
}

public override void SetPreferredJavaSdkPath (string path)
Expand All @@ -162,7 +163,7 @@ public override void SetPreferredJavaSdkPath (string path)
}

javaEl.SetAttributeValue ("path", path);
SaveConfig (doc);
SaveConfig (doc, Logger);
}

public override void SetPreferredAndroidNdkPath (string path)
Expand All @@ -178,22 +179,30 @@ public override void SetPreferredAndroidNdkPath (string path)
}

androidEl.SetAttributeValue ("path", path);
SaveConfig (doc);
SaveConfig (doc, Logger);
}

void SaveConfig (XDocument doc)
bool SaveConfig (XDocument doc, Action<TraceLevel, string> logger)
{
string cfg = UnixConfigPath;
List <string> created = null;

if (!File.Exists (cfg)) {
string dir = Path.GetDirectoryName (cfg);
if (!Directory.Exists (dir)) {
Directory.CreateDirectory (dir);
AddToList (dir);
}
AddToList (cfg);
}
try
{
Directory.CreateDirectory(dir);
}
catch (Exception ex)
{
logger(TraceLevel.Error, $"Unable to create directory {dir}, user do not have the right permissions, {ex.Message}.");
return false;
}
AddToList(dir);
}
AddToList (cfg);
Copy link
Contributor

@jonpryor jonpryor Jun 10, 2019

Choose a reason for hiding this comment

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

This file uses tabs, not spaces, for indentation. All the indentation and associated formatting is wrong.

}
doc.Save (cfg);
FixOwnership (created);

Expand All @@ -203,6 +212,7 @@ void AddToList (string path)
created = new List <string> ();
created.Add (path);
}
return true;
}

static readonly string GetUnixConfigDirOverrideName = $"UnixConfigPath directory override! {typeof (AndroidSdkInfo).AssemblyQualifiedName}";
Expand Down
9 changes: 5 additions & 4 deletions src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,11 @@ protected override string GetShortFormPath (string path)
return KernelEx.GetShortPathName (path);
}

public override void SetPreferredAndroidSdkPath (string path)
{
var regKey = GetMDRegistryKey ();
RegistryEx.SetValueString (RegistryEx.CurrentUser, regKey, MDREG_ANDROID_SDK, path ?? "", RegistryEx.Wow64.Key32);
public override bool SetPreferredAndroidSdkPath (string path)
{
var regKey = GetMDRegistryKey();
RegistryEx.SetValueString(RegistryEx.CurrentUser, regKey, MDREG_ANDROID_SDK, path ?? "", RegistryEx.Wow64.Key32);
return true;
}

public override void SetPreferredJavaSdkPath (string path)
Expand Down