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

Updates to support multiple dotnet targets v3.5, v4.0, v4.5.2, v4.7.2… #415

Closed
wants to merge 11 commits into from
Closed

Conversation

peracto
Copy link
Contributor

@peracto peracto commented Feb 6, 2019

… and dotnetcore2.2

I require dotnetcore2.2 version. While I was in the process, I also added v3.5,v4.0, v4.5.2 and v.4.7.2.
DotnetStandard is a little more complex as it does not fully support System.Drawing.Drawing2D.

In the process I have also updated the tests to support all frameworks.

I have also updated the appveyor.yml script - but don't know how to test it without raising this pull-request. (Is is possible?).

@mrbean-bremen
Copy link
Member

I require dotnetcore2.2 version. While I was in the process, I also added v3.5,v4.0, v4.5.2 and v.4.7.2.

Yes, that would be nice - I have been playing around with netstandard support in an extra branch, but I'm not familiar with it (I'm more of a C++/Python guy and use C# / .NET only occasionally), so it would be helpful if someone with a bit more experience could have a go at it.

I have also updated the appveyor.yml script - but don't know how to test it without raising this pull-request. (Is is possible?).

Yes - you just have to register yourself at AppVeyor using your Github account, and configure it to run for your branches. This is quite straightforward - let me know if you need any help.

@peracto
Copy link
Contributor Author

peracto commented Feb 6, 2019

It's very frustrating. There are a few issues in the configuration. I'll close the PR and test using my new found AppVeyor account.

@peracto peracto closed this Feb 6, 2019
@mrbean-bremen
Copy link
Member

Thanks for looking into this!

@peracto
Copy link
Contributor Author

peracto commented Feb 7, 2019

All tests are now passing. I had to remove net35 from unit testing - I could not get the configuration correct. I have left net35 in the library project - It does compile and pass all unit tests on my local desktop - it was simply an issue with the automated configuration.

As a noob to this whole workflow - I'm still piecing together the whole release cycle. I've been reviewing multiple git/nuget projects, and it seems that there is not a standard method of build-package-release.

For this project, I'm unclear on the nuget process to build and release a package. How is this currently being done ? It does not appear to be in the appveyor CI process. How is the versioning maintained?

Any thoughts and insights would be welcomed.

@peracto peracto reopened this Feb 7, 2019
@mrbean-bremen
Copy link
Member

@tebjan - could you please answer that (e.g. the release process)? I'm also interested in this...

@mrbean-bremen
Copy link
Member

Ah, and thanks a lot - this looks quite promising! I will have another look tonight, but I think we can go this way, and add general netstandard support later.
@tebjan, what do you think?

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

This all looks quite good to me. I added a few remarks and questions, but for me the change looks ok.
There are two things I want to clarify with @tebjan before merging:
EDIT: can be merged as soon as it is ready.

@@ -1,4 +1,5 @@
using System;
#if __EXCLUDED__
Copy link
Member

Choose a reason for hiding this comment

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

Where do you define this? I would have expected this to be defined for .netcore, but I couldn't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was me being lazy. I wanted to keep reference to it - it was a lazy way to comment it out.

@@ -1,393 +1,145 @@
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" ToolsVersion="4.0">
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

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

This has been introduced in MSBuild 15, so I guess we need VS 2017 to build this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are probably correct. It was something that I did not consider when making the changes.

<PackageRequireLicenseAcceptance>true</PackageRequireLicenseAcceptance>
<PackageReleaseNotes>
Supports multiple targets v3.5 thru to dotnetcore2.2.
NetStandard does not fully support the Drawing2D package - so has been left out.
Copy link
Member

Choose a reason for hiding this comment

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

From what I understood, the next (yet unreleased) netstandard 2.1 version will support all of Drawing2D - the interface is already there. I hope that that means that the respective implementations will support this soon, but I couldn't find any specifics as to when this will be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, you are probably correct. I did make an effort to get it working, but rather than spend time hacking a solution, I thought it better to wait for the platform to catch up.
My main focus was to amend the CI/CD process to support multiple targets. Once netstandard catches up, it should simply slot in.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Maybe we don't have to wait long for that.

@@ -1,3 +1,4 @@
#if __EXCLUDED__
Copy link
Member

Choose a reason for hiding this comment

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

so, __EXCLUDED__ actually means "Web included", or did I misunderstand this? Shouldn't the web part be included for .NET implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, me being lazy. There are a number of components that would not compile - and it was easier to remove the whole.
In retrospect, I should leave it enabled for the version of dotnet that support it. I'll review it tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks!

@@ -13,6 +15,32 @@ namespace Svg.UnitTests
public class ImageComparisonTest
{
public TestContext TestContext { get; set; }
#if NETCORE
private static string _basePath = null;
private static string GetSuiteTestsFolder
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this also work for the .NET case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may work - I'll perform the tests tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

That would be nice - I cobbled that test code together to work somehow, but it is not very robust and I don't really like it.

@mrbean-bremen
Copy link
Member

@peracto - are you still working on this? Do you need help here? I think with the few changes you wanted to do, and after clarifying the issue with VS 2017 with @tebjan, we could merge this.

@tebjan - this would be a first step to netstandard compatibility, and the only caveat is that you need VS 2017 to build it (seems to be needed for proper multi-target support). What do think?

@mrbean-bremen
Copy link
Member

@peracto - @tebjan is ok with the need for VS2017, do you need any help to finish this PR (and maybe resolving the conflict)?

@mrbean-bremen
Copy link
Member

@peracto - I have resolved the conflicts and put them into an extra branch, just in case you are working on this one. Please let us know what the state of this PR is - if you don't have the time to work on this, I can just continue.

@bloodwiing
Copy link

@mrbean-bremen Cool. So we can use that one until the branch merges?
If so then could you be kind as to explain on the branched packages installation steps? 😅 Thank you very much.

@mrbean-bremen
Copy link
Member

Sorry, no instructions so far - as you can see above, I also don't know how to package this. I'm just compiling and testing it manually / via CI.
Though I'm going to have a closer look at the packaging - looks like this should not be complicated - but not before the weekend, I'm a bit short on time currently.

@bloodwiing
Copy link

bloodwiing commented Mar 6, 2019

Ah I see. I still appreciate your work and effort 😃

Though I'm going to have a closer look at the packaging - looks like this should not be complicated - but not before the weekend, I'm a bit short on time currently.

Could you tell when that happens? Can't wait 😛

@gvheertum
Copy link
Member

@thedonciuxx @mrbean-bremen
Looking at the source I guess it's just calling a "nuget pack" after you performed a release build. The Nuget folder contains the nuspec file which has the specs for the nuget file to be created. Pushing it to the nuget feed might be a different thing, since that might require an API key or rights on the repository.

@thedonciuxx If you want to quickly use the branched version, I suggest downloading/forking the sources @mrbean-bremen refers to and roll your own build. You will only need the Svg.dll to include in your project. From what I see, no special actions are performed in the package install, just a copy and link of the dll, xml (doc) and pdb (debug) files, so I think waiting for a package is not really necessary.

@mrbean-bremen
Copy link
Member

This is continued in #448, as the owner of this branch is not active anymore, and this had some conflicts that have been difficult to resolve.

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.

5 participants