-
Notifications
You must be signed in to change notification settings - Fork 31
[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.
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
Conversation
…eating directories
…g installation process
| var sdk = CreateSdk(logger); | ||
| bool setSDKPath = sdk.SetPreferredAndroidSdkPath(path); | ||
| return setSDKPath; | ||
| } |
There was a problem hiding this comment.
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
rodrmoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from that, LGTM
| } | ||
|
|
||
| public static void SetPreferredAndroidSdkPath (string path, Action<TraceLevel, string> logger = null) | ||
| public static bool SetPreferredAndroidSdkPath (string path, Action<TraceLevel, string> logger = null) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
| AddToList(dir); | ||
| } | ||
| AddToList (cfg); |
There was a problem hiding this comment.
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.
Approved too hastily, sorry about that. There are still issues here.
|
Closing this PR, this issue will be handled within the installer code. |
Changes: dotnet/android-libzipsharp@1.0.20...1.0.22 * dotnet/android-libzipsharp@9f563dd: Add a test which changes the CompressionMethod of an item (#78) * dotnet/android-libzipsharp@3b610c9: Try actually running the tests under .net core (#77) * dotnet/android-libzipsharp@03cd66b: Archive the SignList.xml * dotnet/android-libzipsharp@8afe791: Add the signListPath parameter * dotnet/android-libzipsharp@ed5585c: Add SignList.xml for signing * dotnet/android-libzipsharp@2ab6943: Add a build_windows.bat file to make it easier to build on windows (#75) * … * dotnet/android-libzipsharp@9dca4fb: Initial commit of localisation * dotnet/android-libzipsharp@668babc: Add Unit Test for setting file permissions on extraction. * dotnet/android-libzipsharp@8f2053c: Build on Windows again (#71)
Partially Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/905583, Need change in installer code, once this is fixed in xamarin-android-tools.
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/905583, VS for Mac installer throws an unauthorized exception, while creating ~/.config files to set sdk path. This PR catches the unhandled exception.
VS for Mac installer needs a return value to check if the SetPreferredAndroidSDKPath was successful, I changed the return value of "SetPreferredAndroidSDKPath" to bool to get a return value. I am not sure the impact of this change in other parts of IDE. Let me know if there are other ways to solve this issue.
A fix for this issue is required, since there are a couple of reports of this unauthorized exception while running installer.