Add support for Mono #82

radical · 2020-01-17T02:07:35Z

on OSX, we get the version from the path
- we return the version of currently running mono, as the first
  result
  - followed by mono with versions from mono --version=number
on linux/windows, we try to get the version from mono --version=number
- if that fails, then we return the instance with 0.0.0.0

- on OSX, we get the version from the path - we return the version of currently running mono, as the first result - on linux/windows, we try to get the version from `mono --version` - if that fails, then we return the instance with `0.0.0.0`

radical · 2020-01-17T02:09:38Z

/cc @akoeplinger

akoeplinger · 2020-01-17T12:04:51Z

src/MSBuildLocator/MSBuildLocator.cs

+                yield break;
+            }
+
+            foreach(var dirPath in Directory.EnumerateDirectories(s_monoOSXBasePath))


do we really needs this? mono --version=number should work fine on OSX.

Switched to using mono --version=number. On OSX, doing this for all the versions found.

src/MSBuildLocator/MSBuildLocator.cs

akoeplinger · 2020-01-17T12:07:26Z

src/MSBuildLocator/MSBuildLocator.cs

+                    if (_isRunningOnMono == null)
+                    {
+                        // There could be potentially expensive TypeResolve events, so cache IsMono.
+                        // Also, VS does not host Mono runtimes, so turn IsMono off when msbuild is running under VS


I assume you're just talking about VS on Windows right?

radical · 2020-02-04T18:24:13Z

@rainersigwald ping

radical · 2020-03-11T19:26:51Z

@rainersigwald while I have your attention, ping ;)

src/MSBuildLocator/DiscoveryType.cs

src/MSBuildLocator/MSBuildLocator.cs

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

Forgind

Would you mind moving the changes to find mono into a separate class/file?

src/MSBuildLocator/MSBuildLocator.cs

Forgind · 2021-01-04T18:10:50Z

Also, per #115 (comment), would you mind pulling in that PR and adding the check?

- move mono specific stuff to `MSBuildLocator.Mono.cs`

.. trailing slash, addressing feedback in microsoft#82 (comment)

Forgind

Also remember #82 (comment), but I think this is good!

src/MSBuildLocator/MSBuildLocator.Mono.cs

Forgind · 2021-01-06T00:21:14Z

src/MSBuildLocator/MSBuildLocator.Mono.cs

+
+                // The path has a valid mono, but we can't find the version
+                // so, let's return the instance at least but with version 0.0.0
+                ver = new Version(0, 0, 0);


Not sure whether this is better or it's better to not return it. Users could find it separately and register it anyway. @rainersigwald, thoughts?

Ok to merge? Or are we waiting for this?

I'd prefer to wait for @rainersigwald's signoff. Also, would you mind adding the if-not-mono check discussed here?

Added, and fixed a bug introduced in the nuget PR.

This is probably ok, let's just listen for feedback.

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Adds an overload to permit supplying additional paths in which to search for MSBuild assemblies beyond the default. Uses this new overload to add the search path for NuGet assemblies for when in a VS install. The latter fixes microsoft#86.

- it was inverted

rainersigwald

Overall LGTM. Thanks for your patience on this!

src/MSBuildLocator/MSBuildLocator.Mono.cs

rainersigwald · 2021-01-07T21:41:32Z

src/MSBuildLocator/MSBuildLocator.Mono.cs

+
+                // The path has a valid mono, but we can't find the version
+                // so, let's return the instance at least but with version 0.0.0
+                ver = new Version(0, 0, 0);


This is probably ok, let's just listen for feedback.

- use RuntimeInformation.IsOSPlatform to detect OSX

radical · 2021-01-08T20:23:36Z

Does <PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" /> look correct?

rainersigwald · 2021-01-13T21:19:35Z

src/MSBuildLocator/Microsoft.Build.Locator.csproj

  </ItemGroup>

  <ItemGroup>
+    <PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" />


Ah, this is unfortunate. I told you to do this! But . . . it's really nice that the Locator package has no dependencies so you can use it basically anywhere. Unfortunately, this type wasn't added to .NET Framework until 4.7.1, so this reference is required to call it in a straightforward way, which makes the package slightly less easy to use.

I think we can call it via reflection instead to avoid that hard requirement--if the type/method is not available, it's not macOS!

Add support for Mono

376fa25

- on OSX, we get the version from the path - we return the version of currently running mono, as the first result - on linux/windows, we try to get the version from `mono --version` - if that fails, then we return the instance with `0.0.0.0`

radical requested review from AndyGerlicher, a2 and rainersigwald and removed request for a2 January 17, 2020 02:07

akoeplinger reviewed Jan 17, 2020

View reviewed changes

Address review comments

3ec1c84

akoeplinger approved these changes Jan 17, 2020

View reviewed changes

JoeRobich reviewed Dec 10, 2020

View reviewed changes

src/MSBuildLocator/DiscoveryType.cs Show resolved Hide resolved

src/MSBuildLocator/MSBuildLocator.cs Outdated Show resolved Hide resolved

src/MSBuildLocator/MSBuildLocator.cs Outdated Show resolved Hide resolved

radical and others added 3 commits December 10, 2020 13:52

Update src/MSBuildLocator/MSBuildLocator.cs

4837872

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

Update src/MSBuildLocator/DiscoveryType.cs

e51852c

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

Remove unnecessary blank lines

f9b158f

Forgind added this to the December 2020 milestone Dec 11, 2020

Forgind suggested changes Dec 21, 2020

View reviewed changes

src/MSBuildLocator/MSBuildLocator.cs Outdated Show resolved Hide resolved

src/MSBuildLocator/MSBuildLocator.cs Outdated Show resolved Hide resolved

rainersigwald mentioned this pull request Jan 4, 2021

Load NuGet assemblies #115

Merged

radical added 4 commits January 5, 2021 14:23

Merge remote-tracking branch 'origin/master' into add-mono-support

06b5deb

Address review feedback

1c5c7bd

- move mono specific stuff to `MSBuildLocator.Mono.cs`

[mono] Handle Directory.EnumerateDirectories returning names with

c5ba85c

.. trailing slash, addressing feedback in microsoft#82 (comment)

little cleanup

5f95f03

radical requested a review from Forgind January 5, 2021 19:50

radical added 2 commits January 5, 2021 14:52

remove blank newline before last closing brace

ebe5226

remove unused usings

3108a59

Forgind approved these changes Jan 6, 2021

View reviewed changes

radical and others added 2 commits January 5, 2021 23:16

address review feedback

e02e0ed

Update src/MSBuildLocator/MSBuildLocator.Mono.cs

241758e

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Forgind approved these changes Jan 6, 2021

View reviewed changes

Forgind and others added 4 commits January 6, 2021 13:19

Load NuGet assemblies (microsoft#115)

7883586

Adds an overload to permit supplying additional paths in which to search for MSBuild assemblies beyond the default. Uses this new overload to add the search path for NuGet assemblies for when in a VS install. The latter fixes microsoft#86.

Fix check for any non existant search paths

f8eb71f

- it was inverted

[mono] Don't include .net framework specific path

04a7fc1

Merge remote-tracking branch 'origin/master' into add-mono-support

b84259d

rainersigwald reviewed Jan 7, 2021

View reviewed changes

rainersigwald approved these changes Jan 7, 2021

View reviewed changes

address review feedback

9794b5a

- use RuntimeInformation.IsOSPlatform to detect OSX

rainersigwald requested changes Jan 13, 2021

View reviewed changes

Forgind closed this Dec 6, 2021

Add support for Mono #82

Add support for Mono #82

Uh oh!

Conversation

radical commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radical commented Jan 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radical commented Feb 4, 2020

Uh oh!

radical commented Mar 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Forgind commented Jan 4, 2021

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radical commented Jan 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

radical commented Jan 17, 2020 •

edited

Loading