GVFS.Upgrader: migrate to netcore project by jamill · Pull Request #1059 · microsoft/VFSForGit

jamill · 2019-04-22T21:16:02Z

GVFS.Upgrader will be run on both Windows and non-Windows platforms.
This enables a netcore version of the project to be built along side a
netframework version.

Update tests to run in the base unit test project to follow the
implementation.

jamill · 2019-05-01T15:19:48Z

+
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <TargetLatestRuntimePatch>true</TargetLatestRuntimePatch>
+    <RuntimeIdentifiers>osx-x64</RuntimeIdentifiers>


This RuntimeIdentifier probably does not belong in this common PropertyGroup that is included for both net461 and netcoreapp2.1.

You can set the identifier conditionally on the TF.

<TargetFrameworks>netcoreapp2.1;net461</TargetFrameworks> <RuntimeIdentifiers>win-x64;osx-x64;linux-x64</RuntimeIdentifiers> <RuntimeIdentifier Condition="'$(TargetFramework)'=='net461'">win-x64</RuntimeIdentifier>

Here when building the TF as net461 you're also forcing the RID to be win-x64.
For TF netcoreapp2.1 you'll need to specify which OS you're targeting still.

What @mjcheetham proposed seems like it should work, but I bet VSMac won't be able to parse it.

It sounds like most of us are moving towards using VSCode going forward, so maybe it's alright to take this proposed change if VSMac can't handle this MSBuild structure.

It sounds like most of us are moving towards using VSCode going forward, so maybe it's alright to take this proposed change if VSMac can't handle this MSBuild structure.

Please don't break VSMac until we've decided that it's the preferred editor for Mac development and have documented how to configure VSCode to build/debug/etc. VFS4G.

Please don't break VSMac

This was my goal. If I couldn't get this to work with a single csproj file, I was going to fall back to having 2 separate csproj files.

alameenshah · 2019-05-08T16:20:06Z

 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
-  <Import Project="$(SolutionDir)\GVFS\GVFS.Build\GVFS.cs.props" />
+  <Import Project="..\GVFS.Build\GVFS.cs.props" />


wondering why this path is changed.

Most likely because Visual Studio like to do things like this 😀 😢

This is because SolutionDir is only defined when building the solution, and is not defined when building the project directly

One thing I've done in the past is to define an MSBuild property <RepoRoot> in a Directory.Build.props file in the root of the repository.

Projects can use $(RepoRoot) if they want to use an absolute path (rather than a maybe more brittle relative one), without needing the Solution build to set $(SolutionDir).

alameenshah

Testing notes -

Getting this error. Should we update the ProductUpgrader.UpgraderToolAndLibs list? Wouldn't the dependent assemblies be different now between netcore and netframework?

> gvfs upgrade --confirm
Checking for GVFS upgrades...Succeeded
New version 1.0.19074.2 is available.
Launching upgrade tool...

ERROR: Could not launch upgrade tool. File copy error - Could not find file 'C:\Program Files\GVFS\Microsoft.Diagnostics.Tracing.EventSource.dll'.

alameenshah · 2019-05-08T16:42:11Z

+  <!-- ItemGroup Conditions are not supported on VS for Mac and Choose/When must be
+       Used instead, see https://github.com/mono/monodevelop/issues/7417 -->
+  <Choose>
+    <When Condition="'$(OS)' == 'Windows_NT'">


The existing pattern to have separate csproj for Mac and Windows (like in GVFS.Mount project) helps avoid the need to do platform checks like this. But your approach of keeping a single project file for both platforms looks cleaner. Also now we end up having 2 different patterns (existing pattern of separate platform specific csproj files and this new common csproj with platform specific source code inclusions.)

It would be good if we could discuss and decide on a single pattern that we could follow consistently. @nickgra, @wilbaker, @kewillford, @jrbriggs - thoughts?

If we can have a single file for both targets, I think that is preferable in the long run. If we can make it work now, I view this as a step in the right direction. At this point, I don't see any blockers in having everything as a single file (debugging on VS for Mac works, I will look again and make sure everything works on VS for Windows).

If there is a problem with using a single file, then my plan was to fall back to having separate projects.

It would be good if we could discuss and decide on a single pattern that we could follow consistently. @nickgra, @wilbaker, @kewillford, @jrbriggs - thoughts?

When @nickgra updated FastFetch to be cross-platform he was able to use with a single csproj (although I believe in that case it's .NET Core on both platforms).

I believe that historically we couldn't do this because the ProjFS nuget package was incompatible with PackageReference. Now that the layout has been fixed we should be able to start switching to using single projects (see also #94).

cc: @nickgra for his thoughts

I think long term we want to move everything to .NET Core and moving to a single project is a good step in that direction. IIRC, PackageReference was what was stopping us from doing this.

If this works fine on VSMac, I'm happy with this solution for now, although it does have the con of making us unable to build Mac on Windows as William called out in another comment.

alameenshah · 2019-05-08T16:59:05Z

The changes look good. A few things to consider before merging.

Weigh the pros & cons of single csproj that includes source files based on platform checks vs existing pattern of keeping separate csprojs.
Should we need to keep different sets of dependent assemblies (in ProductUpgrader.UpgraderToolAndLibs) for netcore and framework targets.
Is ProductUpgrader.TrySetupToolsDirectory() required at all on the Mac? The original problem was that upgrader could not over-write installed assemblies that are open on Windows. This might not be a problem on Mac.

wilbaker

Overall I think the changes are looking good, left a few minor comments.

Please also give @nickgra a chance to review before merging.

wilbaker · 2019-05-13T22:11:13Z

+    <RootNamespace>GVFS.Upgrader</RootNamespace>
+    <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
+
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


As best I can tell AllowUnsafeBlocks was not set before, can this line be removed?

I had thought it was needed when building the project, but apparently not. I will remove it.

wilbaker · 2019-05-13T22:15:23Z

+      <ItemGroup>
+        <ProjectReference Include="..\GVFS.Platform.Mac\GVFS.Platform.Mac.csproj" />
+        <Compile Include="..\GVFS.PlatformLoader\PlatformLoader.Mac.cs">
+          <Link>PlatformLoader.Mac.cs</Link>


I guess this is the one downside to having a single csproj, it means that each platform can only build their own version (at least until we can find an alternative to Condition="'$(OS)').

I'm thinking that might be preferable to doubling the number of projects though.

@jamill @nickgra what are your thoughts?

I guess this is the one downside to having a single csproj, it means that each platform can only build their own version (at least until we can find an alternative to Condition="'$(OS)').

One other thought here, the long term goal is to get everything on .NET Core, and I think switching to single projects is a step in that direction.

I guess this is the one downside to having a single csproj, it means that each platform can only build their own version (at least until we can find an alternative to Condition="'$(OS)').

Another option I was thinking about (not for this PR) might be to condition on the RunTime identifier (or a combination of target framework and runtime identifier).

wilbaker · 2019-05-13T22:16:45Z

@nickgra can you take a look at this PR when you have a few minutes? There aren't too many changes and you have a lot of familiarity in this area. Thanks!

nickgra

Approved with suggestions.

I think this approach is another step in the journey of .NET Core-ifying the product.

nickgra · 2019-05-14T22:24:38Z

+
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <TargetLatestRuntimePatch>true</TargetLatestRuntimePatch>
+    <RuntimeIdentifiers>osx-x64</RuntimeIdentifiers>


What @mjcheetham proposed seems like it should work, but I bet VSMac won't be able to parse it.

It sounds like most of us are moving towards using VSCode going forward, so maybe it's alright to take this proposed change if VSMac can't handle this MSBuild structure.

nickgra · 2019-05-14T22:30:43Z

+  <!-- ItemGroup Conditions are not supported on VS for Mac and Choose/When must be
+       Used instead, see https://github.com/mono/monodevelop/issues/7417 -->
+  <Choose>
+    <When Condition="'$(OS)' == 'Windows_NT'">


I think long term we want to move everything to .NET Core and moving to a single project is a good step in that direction. IIRC, PackageReference was what was stopping us from doing this.

If this works fine on VSMac, I'm happy with this solution for now, although it does have the con of making us unable to build Mac on Windows as William called out in another comment.

GVFS.Upgrader will be run on both Windows and non-Windows platforms. This enables a netcore version of the project to be built along side a netframework version. Update tests to run in the base unit test project to follow the implementation.

jamill requested a review from nickgra April 22, 2019 21:16

jamill force-pushed the upgrader_netcore branch 4 times, most recently from 9f45673 to 25abf19 Compare April 29, 2019 16:29

jamill added this to the M152 milestone May 1, 2019

jamill force-pushed the upgrader_netcore branch 2 times, most recently from b4c80ad to 87c1f87 Compare May 1, 2019 15:17

jamill commented May 1, 2019

View reviewed changes

jamill requested a review from wilbaker May 1, 2019 15:20

jamill marked this pull request as ready for review May 1, 2019 15:20

jamill changed the title ~~Build GVFS.Upgrader for netcore and net framework~~ GVFS.Upgrader: migrate to netcore project May 1, 2019

jamill requested a review from alameenshah May 1, 2019 15:21

jamill commented May 1, 2019

View reviewed changes

Comment thread GVFS/GVFS.Upgrader/GVFS.Upgrader.csproj

jamill force-pushed the upgrader_netcore branch from 87c1f87 to 740604b Compare May 5, 2019 02:36

alameenshah reviewed May 8, 2019

View reviewed changes

Comment thread GVFS/GVFS.UnitTests.Windows/GVFS.UnitTests.Windows.csproj

alameenshah suggested changes May 8, 2019

View reviewed changes

jamill mentioned this pull request May 9, 2019

Upgrader for macOS MVP #1129

Closed

15 tasks

jamill force-pushed the upgrader_netcore branch 2 times, most recently from a379d3d to ad31609 Compare May 13, 2019 19:24

wilbaker reviewed May 13, 2019

View reviewed changes

jamill requested a review from alameenshah May 14, 2019 20:24

jamill force-pushed the upgrader_netcore branch 2 times, most recently from 70a5a39 to d4f971a Compare May 14, 2019 22:16

nickgra approved these changes May 14, 2019

View reviewed changes

alameenshah approved these changes May 15, 2019

View reviewed changes

jamill force-pushed the upgrader_netcore branch from f3ba666 to 7249aa8 Compare May 15, 2019 21:05

GVFS.Upgrader: migrate to netcore project

f9c4bb4

GVFS.Upgrader will be run on both Windows and non-Windows platforms. This enables a netcore version of the project to be built along side a netframework version. Update tests to run in the base unit test project to follow the implementation.

jamill force-pushed the upgrader_netcore branch from 7249aa8 to f9c4bb4 Compare May 15, 2019 23:47

jamill merged commit a6622fb into microsoft:master May 16, 2019

wilbaker added platform: macOS platform: windows labels May 20, 2019

kivikakk mentioned this pull request May 23, 2019

build GVFS.Upgrader on Linux #1191

Closed

jrbriggs modified the milestones: M152, M153 May 23, 2019

Conversation

jamill commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jamill May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alameenshah left a comment

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!

alameenshah commented May 8, 2019

Uh oh!

wilbaker left a comment

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!

Choose a reason for hiding this comment

Uh oh!

wilbaker commented May 13, 2019

Uh oh!

nickgra left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jamill commented Apr 22, 2019 •

edited

Loading

jamill May 10, 2019 •

edited

Loading