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

Add .NET Core 3.1 targeting and package generation #40

Merged
merged 26 commits into from
Dec 17, 2019
Merged

Add .NET Core 3.1 targeting and package generation #40

merged 26 commits into from
Dec 17, 2019

Conversation

erikmav
Copy link
Contributor

@erikmav erikmav commented Nov 26, 2019 •
edited
Loading

NetCore3.1 in VS 16.4 adds C++/CLI support for .NET Core. Add a second 3.1 targeting to the Framework version:

  • Add a NetCore 3.1 target for ProjFS.Managed by factoring out common vcxproj portions and creating two distinct vcxproj files with different targets.
  • Dual-build the test and SimpleProviderManaged projects.
  • New package version translated to SemVer at 1.2.0.0 to supersede existing 1.1.* versions.
  • Update the master build script to properly import the VS 16.4+ environment to make MC++ compilation work.
  • Update the test script to run the test suite under both target frameworks.

Issue #38

wilbaker
wilbaker reviewed Dec 4, 2019
View reviewed changes
@abhijeeg
Copy link

abhijeeg commented Dec 4, 2019

Should we target both .Net Core and .Net Framework as seems to be the case in this PR ? @wilbaker, @cgallred - What's your take on this ?

Upgrade the NetFramework version to 4.7.2 minimum.
Add a NetCore 3.1 target for ProjFS.Managed by factoring out common vcxproj portions and creating two distinct vcxproj files with different targets.

@wilbaker
Copy link
Member

wilbaker commented Dec 4, 2019 •
edited
Loading

Should we target both .Net Core and .Net Framework as seems to be the case in this PR ? @wilbaker, @cgallred - What's your take on this ?

If we target .NET Core and .NET Framework then customers will not be forced to move to .NET Core (though they may need to move to a later version of .NET Framework).

Unless there is a large cost to supporting both I don't see a reason to force people onto .NET Core. @mjcheetham, what are your thoughts?

@mjcheetham
Copy link
Member

If we target .NET Core and .NET Framework then customers will not be forced to move to .NET Core (though they may need to move to a later version of .NET Framework).

Unless there is a large cost to supporting both I don't see a reason to force people onto .NET Core. @mjcheetham, what are your thoughts?

Agreed. If it's not too much effort, keeping a .NET Framework target would be helpful for consumers still targeting Framework.

@erikmav
Copy link
Contributor Author

erikmav commented Dec 8, 2019 •
edited
Loading

RE: NetFramework vs. NetCore: My strong opinion is we should target both. As it stands most projects I'm working on will take years to move to NetCore simply because of the effort needed to move dependencies (like this one!) to Core or Standard one by one then update package references and retest end-to-end. Hundreds of packages at differing Framework vs. Core vs. Standard versions mean a lot of pain. Framework support should never go away in this package until Framework itself is removed from Windows (meaning: effectively never).

@cgallred
Copy link
Contributor

cgallred commented Dec 9, 2019

@Erikma I assume you changed the build script to require >= VS 16.4 because that has .NET Core 3.1, correct? That is what is breaking the validation checks. The Azure Pipelines pool ("Hosted Windows 2019 with VS2019") has "VisualStudio/16.3.6+29418.71". The build script is failing because it can't find a VS 16.4 or greater.

@erikmav
Copy link
Contributor Author

erikmav commented Dec 10, 2019

@cgallred Correct on the 16 4 requirement, it is a strong requirement. I bet the 2019 image gets the update very soon, as I was able to successfully update to netcore31 in another project in mseng and everything was fine. IIRC they usually test there (ring 0) then roll outward.

@cgallred
Copy link
Contributor

cgallred commented Dec 10, 2019 •
edited
Loading

I bet the 2019 image gets the update very soon

@Erikma okay, we'll re-submit the build/test periodically. Once it passes we'll complete the PR.

cgallred
cgallred previously approved these changes Dec 13, 2019
View reviewed changes
@cgallred
Copy link
Contributor

@wilbaker the 2019 Azure image has been updated to VS 16.4 and the tests are now passing. I've approved the PR but I'd appreciate somebody with more .NET experience putting their stamp of approval on it before I merge. :-)

wilbaker
wilbaker reviewed Dec 13, 2019
View reviewed changes
Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

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

@cgallred apologies for the delay, I left a few small follow up questions/comments.

@erikmav
Copy link
Contributor Author

erikmav commented Dec 13, 2019

@wilbaker RE Any CPU: See comment in resolved conversation, we have to leave it since VS locks up on trying to remove. The packaging target needs AnyCPU, and the test and main assembly targets are all explicitly set to x64 in each proj so the sln targeting doesn't matter anyway

@wilbaker
Copy link
Member

@wilbaker RE Any CPU: See comment in resolved conversation, we have to leave it since VS locks up on trying to remove. The packaging target needs AnyCPU, and the test and main assembly targets are all explicitly set to x64 in each proj so the sln targeting doesn't matter anyway

@Erikma thanks! I am having trouble finding the conversation you mentioned, but the details you included in this comment sound good to me.

@cgallred cgallred dismissed their stale review December 13, 2019 22:42

Need to resolve nuspec question.

@cgallred cgallred self-requested a review December 16, 2019 21:57
@erikmav
Copy link
Contributor Author

erikmav commented Dec 17, 2019

@wilbaker Latest commit downgrades to net461 to match previous

@erikmav
Copy link
Contributor Author

erikmav commented Dec 17, 2019 via email

@cgallred
Copy link
Contributor

@wilbaker @jrbriggs @mjcheetham
Erik reverted to Framework 4.6.1. Do these changes look good to you now?

@wilbaker
Copy link
Member

@wilbaker @jrbriggs @mjcheetham
Erik reverted to Framework 4.6.1. Do these changes look good to you now?

These look good to me

@cgallred cgallred merged commit 93be3a6 into microsoft:master Dec 17, 2019
@cgallred cgallred mentioned this pull request Dec 17, 2019
Closed
@cgallred
Copy link
Contributor

@wilbaker @jrbriggs @mjcheetham

FYI this has been uploaded to NuGet.org. Thanks again to @Erikma for doing this work.

Cc: @abhijeeg @PingXie

@cgallred cgallred mentioned this pull request Dec 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrbriggs jrbriggs jrbriggs left review comments

@cgallred cgallred cgallred approved these changes

@wilbaker wilbaker wilbaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@erikmav @abhijeeg @wilbaker @mjcheetham @cgallred @jrbriggs