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.

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

Add dotnet core support #72

Merged
merged 1 commit into from
Jun 6, 2016
Merged

Add dotnet core support #72

merged 1 commit into from
Jun 6, 2016

Conversation

IntelOrca
Copy link
Contributor

@IntelOrca IntelOrca commented May 31, 2016

Adds project.json and necessary #if regions to build for netstandard 1.3.

I am not sure how you want to configure AppVeyor to build this. The new way to create the nuget package that targets both .NET 4.5 and .NET CORE is to do the following:

cd src\HtmlSanitizer
dotnet restore
dotnet build -c Release   # Not actually necessary as pack does this
dotnet pack -c Release    # You can also specify output directory for .nupkg

Other repositories that do dotnet core:

@304NotModified
Copy link
Contributor

304NotModified commented May 31, 2016

}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing "strong naming"?

@IntelOrca
Copy link
Contributor Author

@304NotModified I could, but we still need to run the tests which currently use NUnit. And I believe xunit is the only dotnet test compatible test runner.

@@ -22,6 +24,8 @@
// The following GUID is for the ID of the typelib if this project is exposed to COM
[assembly: Guid("16af04e9-e712-417e-b749-c8d10148dda9")]

#endif

Copy link
Contributor

@304NotModified 304NotModified May 31, 2016

Choose a reason for hiding this comment

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

[assembly: AssemblyVersion("1.0.0.0")] (and maybe others) can be removed isn't it?

Adds project.json and necessary #if regions to build for netstandard 1.3
@mganss
Copy link
Owner

mganss commented Jun 3, 2016

Thanks a lot for the PR. I've pulled your changes to the dotnetcore branch. I had to make a few changes to project.json and add HtmlSanitizer.project.json to get it to build in both VS and the CLI. I've also got the AppVeyor build working (minus the tests). A few points:

  1. NUnit seems to be close to releasing a dotnet test compatible test runner Support dotnet test in .NET CLI and .NET Core nunit/nunit#1371
  2. I created a .NET Core console application, referenced the HtmlSanitizer .NET Core NuGet package, copied the Tests.cs file into the project and ran the tests (except one which depends on the AutoLink package that is not available for .NET Core) using NUnitLite according to this blog post and they are all passing.
  3. I added the relevant strong naming properties to project,json and it builds, though I haven't checked if the resulting assembly is indeed strong named.
  4. The UriFormatException is available in netstandard1.3 (it's in System.Runtime) so I have removed the #if

Still to do:

  • Move the test project to .NET Core
  • Rename the HtmlSanitizer folder to src, HtmlSanitizer.Tests to test
  • Run the tests from AppVeyor once the NUnit test runner for dotnet test is available

Things that are not clear to me yet:

  • Is the NuSpec still necessary (the information it contains is mostly duplicated in project.json now)?
  • Currently, the version information has to be manually adjusted in appveyor.yml and project.json. Can we extract it from project.json in appveyor.yml?
  • Will project.json really be obsolete soon so we'll have to do this all over again?

Thoughts?

@mganss mganss mentioned this pull request Jun 3, 2016
@304NotModified
Copy link
Contributor

304NotModified commented Jun 3, 2016

Is the NuSpec still necessary (the information it contains is mostly duplicated in project.json now)?

No, it even not used ;)

Currently, the version information has to be manually adjusted in appveyor.yml and project.json. Can we extract it from project.json in appveyor.yml?

AFAIK the versions should be at least in project.json and assembly patching won't work.

Will project.json really be obsolete soon so we'll have to do this all over again?

Unclear as most of the .NET Core stuff. We know it's gonna break, but not when, how and sometimes why.

PS if you're waiting on NUnit, why not moving to xUnit? I think the move is really easy in this case

@304NotModified
Copy link
Contributor

304NotModified commented Jun 3, 2016

I added the relevant strong naming properties to project,json and it builds, though I haven't checked if the resulting assembly is indeed strong named.

You can easily check that in the lasted version of NuGet Package Explorer (https://npe.codeplex.com)

image

image

@IntelOrca
Copy link
Contributor Author

IntelOrca commented Jun 3, 2016

Is the NuSpec still necessary (the information it contains is mostly duplicated in project.json now)?

dotnet publish will use project.json instead of a nuspec. So you do not need it anymore.

Currently, the version information has to be manually adjusted in appveyor.yml and project.json. Can we extract it from project.json in appveyor.yml?

Its possible to grab AppVeyor's version from an environment variable, but I don't think you can use them in project.json. You might have to use a script to modify it after checkout. I am not sure what other .NET core repositories do.

Will project.json really be obsolete soon so we'll have to do this all over again?

No idea, Microsoft keep changing their mind about everything every month. It says in that article that they will convert the project.json for you in DEV15 RTM.

@304NotModified
Copy link
Contributor

Currently, the version information has to be manually adjusted in appveyor.yml and project.json. Can we extract it from project.json in appveyor.yml?
Its possible to grab AppVeyor's version from an environment variable, but I don't think you can use them in project.json. You might have to use a script to modify it after checkout. I am not sure what other .NET core repositories do.

Anyway, automatic versions are bad anyway. Very difficult to change them semver compatible.

No idea, Microsoft keep changing their mind about everything every month. It says in that article that they will convert the project.json for you in DEV15 RTM.

Indeed, I'm also how many breaking changes there are between RC and RTM. IMO there should be none, but MS doesn't really understand the terms RC and RTM IMO.

@mganss
Copy link
Owner

mganss commented Jun 5, 2016

I think it's mostly all working now: https://ci.appveyor.com/project/mganss/htmlsanitizer/build/3.3.118

One thing I still don't get are the .xproj files. The test project doesn't have one, only a project.json file, and still everything seems to work. I know .xproj files are only needed for VS but the .sln still references the .csproj files. Do you know how this is supposed to work?

@304NotModified The version number in your screenshot is obviously broken, it's from an intermediate build. But AFAICT it should be a valid version. Both versioning and strong naming work now.

@IntelOrca
Copy link
Contributor Author

IntelOrca commented Jun 5, 2016

@mganss xproj files are just more less a wrapper around the project.json files. You don't need both a .csproj and a .xproj, just an .xproj.

@mganss
Copy link
Owner

mganss commented Jun 6, 2016

OK, I think I have figured it out now. I've added a separate .NET Core .sln that references the .xproj files. You can now either open the "regular" .sln (referencing the .csprojs) and build only for net45/net451 or the .dotnet.sln which builds both net45/1 and netstandard1.3/netcoreapp1.0.

I've also added back OpenCover support using dotnet test.

I'm considering merging to master and releasing a beta NuGet package. What do you think?

@IntelOrca
Copy link
Contributor Author

IntelOrca commented Jun 6, 2016

I'm considering merging to master and releasing a beta NuGet package. What do you think?

That would be good, its the last package I need upgraded to be able to migrate my application to dotnet core.

@mganss mganss merged commit 61de21a into mganss:master Jun 6, 2016
@mganss
Copy link
Owner

mganss commented Jun 6, 2016

Done

@IntelOrca
Copy link
Contributor Author

@mganss just letting you know that I am now using the dotnet core package from NuGet and it appears to working fine in my asp.net core rc2 app!

@tiesont tiesont mentioned this pull request Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants