Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Nov 19, 2018

This is an attempted fix for #2065 and #2062.

System.ValueTuple isn't included with Visual Studio 2015 so we must include it in the VSIX. Visual Studio 2017 includes assembly version 4.0.1.0, but ReactiveUI references assembly version 4.0.3.0. We therefor need to package this assembly.

What this PR does

  • Package the assembly from System.ValueTuple, v2.5.0 NuGet (assembly version 4.0.3.0)

How to test

I think the trick will be installing a version of Windows that doesn't have .NET 4.7 installed (Windows 10 will automatically install this).

Windows version compatibility

The earliest version of Windows that Visual Studio 2017 supports is:

Windows 7 Home Premium can be found here:

The earliest version of Windows that Visual Studio 2015 works with is Windows XP, see:

Testing with Windows XP is maybe overkill? 😉 I guess we should decide what the lowest Windows version we want to support is?

Visual Studio 2015

  1. Install Visual Studio 2015 on Windows 7
  2. Install GitHub for Visual Studio
  • Check login dialog works

Visual Studio 2017

  1. Install Visual Studio 2017 on Windows 7
  2. Install GitHub for Visual Studio
  • Check login dialog works

User feedback

This build is working as intended. Thanks a lot!
#2062 (comment)

Was using Windows LTSB.

When I press "Connect to GitHub", the form appeared. But clicking on the tab "GitHub Enterprise" (to choose the url to our GitHub Enterprise on premise), did not work
#2065 (comment)

We think this is an unrelated issue.

No response from user on Halp.

Related

  • When i click "Connect" in the GitHub section at TeamExplorer pane, Visual Studio crashes (Halp)

Fixes #2065
Fixes #2062

<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<IncludeInVSIX>true</IncludeInVSIX>
</Content>
<Content Include="System.ValueTuple.dll">
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this approach of adding the assembly to the repo seems a little odd. Why not just add the nuget package with the proper version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find the rules for when a file is actually included in the .vsix to be super confusing.

It seems like all of the following are required:

  1. Appear as Asset in .vsixmanifest
  2. Appear as Content in .csproj
  3. IncludeInVSIX=true
  4. CopyToOutputDirectory

I hoped I'd be able to simply add Assets to .vsixmanifest that point to files in the output directory. Alas this doesn't seems to work (the extra ceremony is required). 😕

I've updated the project to using a Link rather than including the binary. This is maybe a little better, but still not great. Let me know if you find a cleaner way!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. I thought adding the NuGet package would automatically include the assembly from it in the VSIX.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I hoped as well, bit it looks like there are differences between a plain old assembly reference and a <PackageReference>. I ended up with missing assemblies when I converted TestDriven.Net to use <PackageReference> as well. 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought this was fixed: NuGet/Home#5899 You might want to report a bug if you still encounter this in recent versions.

@codecov
Copy link

codecov bot commented Nov 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (release/2.5.10@3729292). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                @@
##             release/2.5.10    #2068   +/-   ##
=================================================
  Coverage                  ?   39.31%           
=================================================
  Files                     ?      410           
  Lines                     ?    17585           
  Branches                  ?     2435           
=================================================
  Hits                      ?     6913           
  Misses                    ?    10120           
  Partials                  ?      552

@jcansdale jcansdale changed the base branch from master to release/2.5.9 November 20, 2018 10:24
@jcansdale jcansdale changed the base branch from release/2.5.9 to release/2.5.10 November 20, 2018 10:29
System.ValueTuple isn't included with Visual Studio 2015 so we must
include it in the .vsix.
Link it from NuGet folder rather than include binary in project.
@jcansdale jcansdale force-pushed the fixes/2062-package-ValueTuple branch from 22eb520 to 858470c Compare November 21, 2018 18:11
@jcansdale jcansdale merged commit 946c0db into release/2.5.10 Nov 21, 2018
@jcansdale jcansdale deleted the fixes/2062-package-ValueTuple branch November 21, 2018 18:12
@jcansdale jcansdale restored the fixes/2062-package-ValueTuple branch November 21, 2018 18:57
@jcansdale jcansdale deleted the fixes/2062-package-ValueTuple branch November 23, 2018 13:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants