Feature/add vs2017 support #1056
Conversation
|
It looks like the latest Paket dependencies were not retrieved for the AppVeyor build? |
|
Thanks for the pull request! I will have a look for the paket problem (I hope their is no need to install VS 2017 because I don't have installed it at the moment :-()
I'm sorry to tell you that it's the main thing to test :-( You should absolutely test a check in with a check in policy against a Tfs 2017 to be sure it works well. I hope you could do that... |
GitTfs.Setup/GitTfs.Setup.csproj
Outdated
| <Reference Include="WixSharp"> | ||
| <HintPath>..\packages\WixSharp\lib\WixSharp.dll</HintPath> | ||
| <Reference Include="WixSharp.UI"> | ||
| <HintPath>..\packages\WixSharp\lib\WixSharp.UI.dll</HintPath> |
There was a problem hiding this comment.
Looks like just a little noise in the diff... not that it's critical, but it appears this and the section above were simply 'swapped' with no material changes unless I'm overlooking something
There was a problem hiding this comment.
Yup. And VS kept doing this--so I just decided to check it in to stop the git noise.
paket.lock
Outdated
| WixSharp (1.0.22.3) | ||
| xunit (2.1) | ||
| xunit.assert (2.1) | ||
| xunit.core (2.1) |
| # Visual Studio 2012 | ||
| # Visual Studio 15 | ||
| VisualStudioVersion = 15.0.26228.12 | ||
| MinimumVisualStudioVersion = 10.0.40219.1 |
There was a problem hiding this comment.
Curiosity - did you have to manually define the minimum or was vs17 kind enough to figure it out for you?
There was a problem hiding this comment.
When I saw this in the diff, it looks like VS2017 figured this out--which is pretty cool.
GitTfs.Vs2015/GitTfs.Vs2015.csproj
Outdated
| @@ -139,7 +139,7 @@ | |||
| </When> | |||
| </Choose> | |||
| <Choose> | |||
| <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.5'"> | |||
| <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And ($(TargetFrameworkVersion) == 'v4.5' Or $(TargetFrameworkVersion) == 'v4.5.2')"> | |||
There was a problem hiding this comment.
More curiosity, why did we need to support 4.5.2? Any particular you're depending on?
There was a problem hiding this comment.
The way to detect an installed instance of Visual Studio 2017 required a NuGet package reference (though, this project uses Paket), and that package required .Net 4.5.2.
| return build != null ? build : queuedBuild.Build; | ||
| } | ||
|
|
||
| #pragma warning disable 618 |
There was a problem hiding this comment.
Typically I like to leave a code comment as to why I'm suppressing a warning, in this case, an obsolete warning
There was a problem hiding this comment.
I know it says I added it, but this code was actually copied from another file which also contained this warning suppression (from the GitTfs.Vs2015 project). I merely used the GitTfs.Vs2015 project as a template, and then customized it for the required changes to support Visual Studio 2017.
| xunit.core (2.2) | ||
| xunit.abstractions (2.0.1) - framework: net452 | ||
| xunit.assert (2.2) | ||
| xunit.core (2.2) |
There was a problem hiding this comment.
whoa, was it intentional to upgrade all the packages in this commit? Do we need them for something?
There was a problem hiding this comment.
Unlike NuGet, unless you specify otherwise, Paket will get the latest version of the dependencies.
| @@ -697,7 +697,7 @@ public bool Checkout(string commitish) | |||
| { | |||
| try | |||
| { | |||
| _repository.Checkout(commitish); | |||
| LibGit2Sharp.Commands.Checkout(_repository, commitish); | |||
There was a problem hiding this comment.
hmm, looks like they did away with the extension method, or was this a style preference?
There was a problem hiding this comment.
According to the docs, the Checkout method is deprecated and LibGit2Sharp.Commands.Checkout(...) is the replacement.
There was a problem hiding this comment.
Right you are. The change is here. Appears to be a style choice on the dev to not use extension methods.
@pmiossec I totally agree. I just wanted to get some eyes on this. I hope that I will have some time to test this soon. I'll report back as soon as I have made this test. Of course, there are others on #1054 who are waiting for this support, so perhaps they could test this PR, too. That would be very helpful. |
|
@fourpastmidnight I failed to fix the build on AppVeyor but there is some good fixes that you could find in my branch: https://github.com/pmiossec/git-tfs/tree/pull/1056 Cherry pick if you want...
The project VS2017 still fails to build and I don't know why at the moment (but that's time to sleep ;) ) |
👍 |
fourpastmidnight
force-pushed
the
feature/add-vs2017-support
branch
from
353c0e5
to
d3ec9f5
Compare
|
I made sure the new VS2017 project built as x86 architecture for Debug builds, but did not do the same for Release builds. |
|
@pmiossec OK, I rebased your branch, However, I looked at the TeamCity build log, and it appears the build server does not have the .Net Framework 4.6.2 installed, and so of course, the solution now cannot be built on this build server. I believe the problem with AppVeyor is the same. The NuGet package If that's possible to do, then we should be good to go. |
Don't worry about the TeamCity build. It's been less reliable than the AppVeyor build for a while, and I've been meaning to disable or remove it. (Edit: I disabled the build trigger in teamcity, so its status should stop showing up for new commits.) |
GitTfs.sln
Outdated
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| Debug|Any CPU = Debug|Any CPU |
There was a problem hiding this comment.
Can you remove the extra build configuration? It looks like it's mostly reusing project build configurations.
Having just an "x86" build configuration was important in the past, because some of the older TFS client libs are x86-only, so an AnyCPU build would sometimes run in 64-bit mode and then crash when loading the 32-bit DLLs, or something like that. At some point in the future we'll switch everything to be an "Any CPU" build, just not now.
There was a problem hiding this comment.
Hmm, missed that one. No problem.
| protected override bool HasWorkItems(Changeset changeset) | ||
| { | ||
| return Retry.Do(() => changeset.AssociatedWorkItems.Length > 0); | ||
| } |
There was a problem hiding this comment.
This method is defined in TfsHelperVs2012Base. Would it make sense to have this TfsHelper class inherit from TfsHelperVs2012Base instead of from TfsHelper? The intention for TfsHelperVs2012Base was that it would be a good starting point for VS2012 and everything after that.
There was a problem hiding this comment.
Yep, I originally started out inheriting from that class, but much has changed between VS2012-2015 that this base class doesn't make sense to use for 2017. For example, there are method signature changes in version 15 of the Team Foundation client libraries which are breaking changes from version 14.
Also, I'm going to put this PR on hold. While I think checking in will work, my organization uses a comment check-in policy, and I need to find a way to get that in git-tfs; but the current architecture of git-tfs made an assumption (not necessarily a bad one, but unfortunately things changed) that VS' installation folder structure would remain nearly constant--and that's simply not the case with VS2017. And because of the current architecture, I'm unable to change the event handler for assembly resolution because it's buried in the base class constructor.
Having said all of that, I hope at least we can get .Net 4.6.2 on the AppVeyor build server so that when this PR is fully ready, it can be built, tested, and merged.
There was a problem hiding this comment.
You know, I was thinking about this, and I think I can simplify the Paket Dependencies. Because @pmiossec updated all the projects to compile against .NET 4.6.2, there's no reason for the VS2015 plugin to be "locked down" to using the v14 Team Foundation client libraries. That plugin could also make use of the v15 libraries. This would eliminate the need for the "paket dependency groups" I created. But, this would also mean that the 2015 plugin would not be able to make use of the TfsHelperVs2012Base class because of the breaking changes in method signatures introduced in v15 of the Team Foundation client libraries.
fourpastmidnight
changed the title
|
Officially putting this on hold so I can prepare git-tfs to more completely support VS2017. |
fourpastmidnight
changed the title
fourpastmidnight
force-pushed
the
feature/add-vs2017-support
branch
from
e0a3072
to
c1ec06d
Compare
|
The latest push added the internal This PR still needs some work, however. For one thing, I need to make some changes for when |
The availability of the nuget packages for TFS client libraries has made it tempting for me to change the version-based stuff in git-tfs. Git-tfs could bundle the latest client libraries, which should be able to access whichever (not too ancient) TFS server you're using. That's a clear win, since the way that git-tfs bundles support for all the client libraries and then picks one at runtime is a big kludge. Before the nuget packages, git-tfs didn't include the TFS DLLs in its distribution, so it was kind of a necessary evil. (The alternative was to build and distribute git-tfs separately for each supported version of the client libraries, which IMO would have been worse.) The reason that I'd hesitate to remove all of the per-TFS-version stuff is checkin policies: do those still need to be paired with a specific set of client libraries? If that's the case, it would probably make sense to stop bundling TFS client libraries with git-tfs at all, so that git-tfs can pick the right client libs based on what's installed on the local machine. |
I kind of feel the same way about this--especially now in light of VS2017 and the significant changes with it compared to previous VS versions. But, I have no way of testing such a change, personally, so I couldn't put up a PR "hoping for the best" with such changes. It would be nice, though, to see if it would work.
I don't believe that's the case. In my situation, my changes are not working for me in my organization because it's looking for v10.0.0.0 of the The biggest unknown, is whether or not using only the latest Team Foundation client libraries with git-tfs would work with older versions of TFS, especially TFS 2010 through TFS 2013. |
Which versions of VS allow you to check in with this checkin policy?
I found a compatibility matrix for clients up to 2015 and servers up to 2017, and it looks like git-tfs could use the VS2015 client libs only and still access TFS servers back to 2010. That's pretty good, but the biggest outstanding question is still checkin policies. If a project is configured with a VS2012 checkin policy, what happens when you check in from VS2015 or VS2017? If it works, can git-tfs be made to do the same thing? |
With version 15 of the Microsoft.TeamFoundationServer.* NuGet packages, some of the types and method signatures that were available in 14.x have changed or been removed. In order to attempt to preserve maximum compatibility with VS2015, while adding support for VS2017, a dependency group has been introduced into the `paket.dependencies` file called VS2015. The corresponding VS2015 `paket.references` file has been updated to reference this group. A future commit will introduce a VS2017 dependency group. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
Because of the differences in installation folder layout between Visual Studio 2010-2015 and the latest Visual Studo 2017, it is necessary to allow each GitTfs Helper to define how to load an assembly from the Visual Studio folder. This was accomplished by creating a private `_LoadFromVsFolder` method which calls the (now) protected and abstract `LoadFromVsFolder` method. Each GitTfs Helper must now implement/override this method. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
Adds a new GitTfs.Vs2017 project and the appropriate references in a paket.references file to add Visual Studio 2017 support to git-tfs. This commit also bumps the version of git-tfs up to 0.26.0. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
This commit links in the common files that the other VS* projects link to, and also adds specific support for Visual Studio 2017. The only file that is not linked in is the TfsHelper.Vs2012Base.cs file. This is because Visual Studio 2017 is drastically different than Visual Studio 2012-2015 in terms of its installation and how you find information about installed Visual Studio instances. Therefore, TfsHelper.Vs2017.cs inherits directly from TfsHelperBase directly, and overrides the required methods. Visual Studio 2017 does not install many things into the Windows Registry. There is guidance online that details how to determine the installation path of an instance of Visual Studio 2017. (There can be many instances of Visual Studio 2017 installed on any one machine now.) This guidance has been used to determine the installation path, which in turn is used to find the DLL containing the TFS Checkin Form. The TFS Checkin Form is still in a DLL which is part of a Visual Studio extension, but the extension now has a deterministic installation location within the Visual Studio 2017 installation path. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
Removes the 'Any CPU' build configuration from the solution, per @spraints request. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
NCrunch is a continuous unit test runner which provides developers with a tight feedback loop by running impacted unit tests upon detecting changes to the underlying source code. NCrunch recommends that these files be checked into source code control--they configure the projects and solution to work with NCrunch. Normally, getting NCrunch to work with a solution is easy, just enable it. But for Git-Tfs, it was a bit more complicated because of the architecture and because of LibGit2Sharp's unmanaged dependency. So, checking in these files will ensure that any other developers using NCrunch will be all set to go. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
When installing the Paket depenencies, the latest version of LibGit2Sharp was installed. There was at least one obsolete method that was being used; it has been updated to use the replacement. Signed-off-by: Craig E. Shea <craig.e.shea@gmail.com>
When I branched off of master, there were 5 broken unit tests. The main issue was that a branch named "branch" was created, and then a commit would be made and a comparison of the resulting SHAs. However, what was forgotten in these tests was to checkout the branch "branch" after it was created prior to creating the next commit. These oversights were corrected. Also, with the new version of LibGit2Sharp, the unit tests needed some updating to remove method calls to obsolete methods. Also updated the ignore file to ignore NCrunch, Resharper, and DevTracker files. Signed-off-by: Craig E. Shea <crag.e.shea@gmail.com>
Solve on AppVeyor (?) "The framework restriction Paket.Requirements+FrameworkRestrictions+_AutoDetectFramework and Paket.Requirements+FrameworkRestrictions+FrameworkRestrictionList could not be combined."
fourpastmidnight
force-pushed
the
feature/add-vs2017-support
branch
from
c1ec06d
to
66dbe36
Compare
|
I let that here if it could help (a nuget package to locate >=VS2017 and hopefully get VS install dir): |
|
Any plans to finish the VS2017 support and get it merged back into the main/trunk? |
|
@fourpastmidnight thank you for work on this PR. Me and my collegues have faced the same issue with unsupported VS17. Are there any updates? Maybe we can help you? |
Adds preliminary support for Visual Studio 2017 (see #1054). As far as I know, this works. All unit tests pass, as do the functional/smoke tests. In addition, I was able to synchronize my local git branch with my organization's TFS server. I have not, however, tried to check-in anything as of yet.
I expect some comments to come back on this, so please do. I look forward to making any required adjustments to this PR so that we can get this into the product.