Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove dev14 references in source code #2341

Merged
merged 7 commits into from
Nov 14, 2018
Merged

Remove dev14 references in source code #2341

merged 7 commits into from
Nov 14, 2018

Conversation

PatoBeltran
Copy link
Contributor

@PatoBeltran PatoBeltran commented Jul 12, 2018 •
edited
Loading

Bug

Fixes: NuGet/Home#7112

Fix

Details: Remove all the scripts, hacks, ifdefs and tests for the now-unsupported dev14.

@PatoBeltran PatoBeltran self-assigned this Jul 12, 2018
@PatoBeltran PatoBeltran force-pushed the dev-pb-kill-dev14 branch 2 times, most recently from 55a6ef0 to 9d711fa Compare July 12, 2018 21:27
@nkolev92
Copy link
Member

nkolev92 commented Jul 17, 2018 •
edited
Loading

@PatoBeltran
This is a WIP?
Will you update it again? Ready for review?

nkolev92
nkolev92 approved these changes Jul 17, 2018
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a 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.

build/common.project.props
<PropertyGroup Condition="'$(VisualStudioVersion)' == '14.0'">
<MinimumVisualStudioVersion>14.0</MinimumVisualStudioVersion>
<DefineConstants>$(DefineConstants);VS14</DefineConstants>
<VSSDKRoot>$(RepositoryRootDirectory)packages\Microsoft.VSSDK.BuildTools.14.3.25420</VSSDKRoot>
Copy link
Member

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.

src/NuGet.Clients/NuGet.VisualStudio.Common/ProjectHelper.cs Outdated
#if VS14
using Microsoft.VisualStudio.ProjectSystem.Designers;
#elif VS15
#if VS15
using Microsoft.VisualStudio.ProjectSystem.Properties;
#endif
Copy link
Member

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?

Copy link
Contributor Author

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?

test/EndToEnd/tests/InstallPackageTest.ps1 Outdated
# Assert
Assert-Package $p "jQuery"
}

function Test-InstallPackageIntoNativeWinStoreApplication {
[SkipTestForVS15()]
Copy link
Member

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.

@nkolev92
Copy link
Member

@PatoBeltran
Are you still digging into this?

//cc @NuGet/nuget-client can someone else please take a look.

@PatoBeltran PatoBeltran changed the title WIP: remove dev14 stuff Remove dev14 references in source code Jul 17, 2018
jainaashish
jainaashish reviewed Jul 18, 2018
View reviewed changes
...Get.Clients/NuGet.PackageManagement.VisualStudio/NuGet.PackageManagement.VisualStudio.csproj Outdated
<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'">
Copy link
Contributor

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

jainaashish
jainaashish reviewed Jul 18, 2018
View reviewed changes
...Get.Clients/NuGet.PackageManagement.VisualStudio/NuGet.PackageManagement.VisualStudio.csproj Outdated
<HintPath>$(MSBuildToolsPath)\Microsoft.Build.dll</HintPath>
<Private>False</Private>
</Reference>
</ItemGroup>
<ItemGroup Condition="'$(VisualStudioVersion)' == '15.0'">
Copy link
Contributor

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

jainaashish
jainaashish approved these changes Jul 18, 2018
View reviewed changes
Copy link
Contributor

@jainaashish jainaashish left a 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.

@PatoBeltran
Copy link
Contributor Author

@NuGet/nuget-client is this something we want to leave as is? https://github.com/NuGet/NuGet.Client/pull/2341/files#diff-46b8b89f0b5ffc7c8be0713918765ce6R51

@nkolev92
Copy link
Member

I think it's ok for now.

Step 1 is removing dev 14....step 2 will be making it Dev16 friendly.

@PatoBeltran
Copy link
Contributor Author

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

@nkolev92
Copy link
Member

@PatoBeltran

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 :)

@nkolev92
Copy link
Member

@PatoBeltran
I've rebased from dev here.

We should probably merge this soon :)

@PatoBeltran
Copy link
Contributor Author

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.

@nkolev92 nkolev92 merged commit 1d25f30 into dev Nov 14, 2018
@nkolev92 nkolev92 deleted the dev-pb-kill-dev14 branch November 14, 2018 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@jainaashish jainaashish jainaashish approved these changes

@rohit21agrawal rohit21agrawal Awaiting requested review from rohit21agrawal

@zhili1208 zhili1208 Awaiting requested review from zhili1208

@mishra14 mishra14 Awaiting requested review from mishra14

@AbdallahAbuHashem AbdallahAbuHashem Awaiting requested review from AbdallahAbuHashem

Assignees

@PatoBeltran PatoBeltran

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PatoBeltran @nkolev92 @jainaashish