Skip to content

Support for finding Xamarin.Android location with only VS 2017 installed#3

Merged
dellis1972 merged 9 commits intodotnet:masterfrom
jonathanpeppers:maybe-2017
Jun 26, 2017
Merged

Support for finding Xamarin.Android location with only VS 2017 installed#3
dellis1972 merged 9 commits intodotnet:masterfrom
jonathanpeppers:maybe-2017

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jun 22, 2017

  • Included a unit test project with smoke tests (I just used 2.x NUnit). These are passing on my Windows machine with VS 2017.
    - Uses vswhere to locate a VS 2017 installation that includes Xamarin. I think this is the preferred method for locating VS 2017 now.
  • Looks for VS 2017 in known locations

Not a fan of this, maybe a registry key would be better?
This is basically just a smoke test making sure all the paths exist, and
`AndroidLogger` doesn't throw errors out.
- We can run `vswhere -latest -requires Component.Xamarin` which will
locate the latest VS installation that contains Xamarin
- Using `-property installationPath` causes stdout to only contain the
single value
- Included comments containing various URLs of interest
@jonpryor
Copy link
Contributor

I have one conceptual problem with this: there can be multiple Visual Studio installations on a single machine.

For example, I have VS 2017 Enterprise and Community installed. When I run vswhere, I get:

> C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise>..\..\Installer\vswhere -latest -requires Component.Xamarin -property installationPath
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community

i.e. it returns the product I most recently installed, because of -latest. If I remove -latest, I get all installations:

> C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise>..\..\Installer\vswhere -requires Component.Xamarin -property installationPath
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise

The question is thus: how should xamarin-android-tools know which one is the appropriate and relevant version to use?

I am not sure that can be answered.

- First check %VSINSTALLDIR% if we are in VS command prompt
- Next check each known edition: Enterprise, Professional, and Community
@jonathanpeppers
Copy link
Member Author

@dellis1972 @jonpryor

I switched to probing directories. I tried @grendle's code from the installer, but it didn't include support for 2017. I also noticed they have removed the registry key in 2017 in favor of vswhere...

I think this solution is the most reliable for now.

(File.Exists (Path.Combine (loc, DebugRuntime)) || // Normal/expected
File.Exists (Path.Combine (loc, ClassParseExe)) || // Normal/expected
File.Exists (Path.Combine (loc, "Ionic.Zip.dll"))); // Wrench builds
File.Exists (Path.Combine (loc, "Ionic.Zip.dll")) || // Wrench builds
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this check, as we shouldn't even be using Ionic.Zip.dll anymore!

return monoAndroidPath;
}

string xamarinSdk = Path.Combine (OS.ProgramFilesX86, "MSBuild", "Xamarin", "Android");
Copy link
Contributor

Choose a reason for hiding this comment

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

There's another problem here: xamarinSdk is the VS 2015 install location. VisualStudioPaths contains VS 2017 locations.

It is possible to have both VS 2015 and VS 2017 installed.

Should we really prefer the VS 2015 install location over the VS 2017 install location, if both are installed? I think we should check for VS 2015 after VS 2017 locations.

Additionally, this could be cleaned up by "moving" xamarinSdk into VisualStudioPaths, so that they're treated uniformly.

if (string.IsNullOrEmpty(vsPath))
continue;
xamarinSdk = Path.Combine(vsPath, "MSBuild", "Xamarin", "Android");
if (Directory.Exists(xamarinSdk) && ValidateRuntime(xamarinSdk))
Copy link
Contributor

Choose a reason for hiding this comment

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

Coding convention: space before (.

- Also check for them last, choosing 2017 first
- Fixed coding style
The code below actually appendsd \MSBuild\Xamarin\Android, so we
shouldn't include it here.
@dellis1972 dellis1972 merged commit 6a81bcb into dotnet:master Jun 26, 2017
rmarinho added a commit that referenced this pull request Mar 13, 2026
Fix #1: Validate avdName in LaunchAvd (ArgumentException on null/empty)
Fix #2: RunShellCommandAsync now returns full trimmed stdout (not just first line)
Fix #3: Add 9 FirstNonEmptyLine parsing tests + 3 LaunchAvd validation tests
Fix #5: Log stderr via logger on shell command failures (AdbRunner gets logger param)
Fix #6: Remove TOCTOU HasExited check in TryKillProcess (rely on catch block)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rmarinho added a commit that referenced this pull request Mar 13, 2026
Fix #1: Validate avdName in LaunchAvd (ArgumentException on null/empty)
Fix #2: RunShellCommandAsync now returns full trimmed stdout (not just first line)
Fix #3: Add 9 FirstNonEmptyLine parsing tests + 3 LaunchAvd validation tests
Fix #5: Log stderr via logger on shell command failures (AdbRunner gets logger param)
Fix #6: Remove TOCTOU HasExited check in TryKillProcess (rely on catch block)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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