-
Notifications
You must be signed in to change notification settings - Fork 693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dev14 references in source code #2341
Conversation
55a6ef0
to
9d711fa
Compare
@PatoBeltran |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of minor suggestions, but looks good overall.
It's a great start.
<PropertyGroup Condition="'$(VisualStudioVersion)' == '14.0'"> | ||
<MinimumVisualStudioVersion>14.0</MinimumVisualStudioVersion> | ||
<DefineConstants>$(DefineConstants);VS14</DefineConstants> | ||
<VSSDKRoot>$(RepositoryRootDirectory)packages\Microsoft.VSSDK.BuildTools.14.3.25420</VSSDKRoot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the package from the packages.config that is used during our bootstrap.
#if VS14 | ||
using Microsoft.VisualStudio.ProjectSystem.Designers; | ||
#elif VS15 | ||
#if VS15 | ||
using Microsoft.VisualStudio.ProjectSystem.Properties; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this #endif need removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still have a #if VS15
here, should we remove that one as well?
# Assert | ||
Assert-Package $p "jQuery" | ||
} | ||
|
||
function Test-InstallPackageIntoNativeWinStoreApplication { | ||
[SkipTestForVS15()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one probably needs removed too.
Find the references of SkipTestForVS15 and remove them.
@PatoBeltran //cc @NuGet/nuget-client can someone else please take a look. |
c1f79c2
to
df33fd7
Compare
<HintPath>$(SolutionPackagesFolder)Microsoft.VisualStudio.ProjectSystem.14.1.127-pre\lib\net451\Microsoft.VisualStudio.ProjectSystem.Interop.dll</HintPath> | ||
<EmbedInteropTypes>True</EmbedInteropTypes> | ||
</Reference> | ||
</ItemGroup> | ||
<ItemGroup Condition="'$(VisualStudioVersion)' == '15.0'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these conditions as well since this should apply for dev16 by default until we start to have separate dependencies
<HintPath>$(MSBuildToolsPath)\Microsoft.Build.dll</HintPath> | ||
<Private>False</Private> | ||
</Reference> | ||
</ItemGroup> | ||
<ItemGroup Condition="'$(VisualStudioVersion)' == '15.0'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above and every other place which has this condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good 👏 Just remove dev15 condition for packageReferences then we should be good for dev16 as well.
@NuGet/nuget-client is this something we want to leave as is? https://github.com/NuGet/NuGet.Client/pull/2341/files#diff-46b8b89f0b5ffc7c8be0713918765ce6R51 |
I think it's ok for now. Step 1 is removing dev 14....step 2 will be making it Dev16 friendly. |
if you do a search for "14.0", "14" or "dev14" there are still a couple of things which I'm not sure if we should delete |
3d5ba86
to
6006504
Compare
We can start by merging this PR, and then have a new one for those changes/discuss them in the above issue/new one? Let's not dwell on it for too long :) |
6006504
to
4043d18
Compare
@PatoBeltran We should probably merge this soon :) |
There are still a bunch of functional test failures that we have to take care of before merging this, we should investigate on this and merge it soon. |
4043d18
to
05b029a
Compare
Bug
Fixes: NuGet/Home#7112
Fix
Details: Remove all the scripts, hacks, ifdefs and tests for the now-unsupported dev14.