Skip to content
This repository was archived by the owner on Jun 27, 2019. It is now read-only.

Port to NETStandard #40

Closed
dazinator opened this issue Jun 28, 2017 · 17 comments · Fixed by #42
Closed

Port to NETStandard #40

dazinator opened this issue Jun 28, 2017 · 17 comments · Fixed by #42

Comments

@dazinator
Copy link
Member

dazinator commented Jun 28, 2017

Feature description

Port the solution NETStandard.
This will unlock ability to port GitVersionTask to .NETStandard.
This will unlock dotnet cli builds for projects that consume GitVersionTask

Here is the API Portability analysis: ApiPortAnalysis.xlsx

@dazinator
Copy link
Member Author

dazinator commented Jun 28, 2017

Need latest libgit2sharp nuget package (with supports for netstandard1.3) to be released.

@dazinator
Copy link
Member Author

dazinator commented Jun 28, 2017

I have started work in a branch but it's not yet successfully building.

  1. Moved all the code into a netstandard 1.3 project, and deleted the net4, net45 and shared projects.
  2. Tried to resolve all compilation errors, by adding relevant dependencies back, however some aren't yet available..
  3. Added a polyfill for serialization attribute.. not sure if that is the correct way to handle this..

Following cannot be resolved:

  1. LibGit2Sharp (awaiting nugget release with netstandard 1.3 support)
  2. [StringFormatMethod("format")] attribute (no idea how to replace this - perhaps need compilation directive.)
  3. using JetBrains.Annotations;
  4. SerializationInfo - (no idea how to replace this - perhaps need compilation directive.)

@dazinator
Copy link
Member Author

Have attached an API portability analysis: ApiPortAnalysis.xlsx

@pdrhsm
Copy link
Contributor

pdrhsm commented Jun 30, 2017

@dazinator
2. StringFormatMethod is just a ReSharper helper-attribute as far as I know.
3. Latest version, 10.4.0, supports netstandard1.0-- or one could remove the annotations.
4. Just took a quick look,.. I can't see that WarningException ever gets serialized, but I might be missing something.

@dazinator
Copy link
Member Author

Yeah the serialisation constructor for exceptions used to be a best practice. Im not sure its required anymore though..

@conniey
Copy link

conniey commented Jun 30, 2017

[StringFormatMethod("format")] attribute (no idea how to replace this - perhaps need compilation directive.)
using JetBrains.Annotations;

I would add a compiler directive for these ReSharper features based on the platform you are currently viewing in the editor, so at least if they switch to the full .NET Framework, these features will still work for them. (ie. #if NET45)
[Edit: never mind from @pdrhsm 's comment it is supported on .NETStandard 1.0 now]

image

SerializationInfo - (no idea how to replace this - perhaps need compilation directive.)

Since binary serialization is not supported until .NET Standard 2.0, I have been using compiler directives to comment this feature out

@JakeGinnivan
Copy link
Contributor

Happy to just remove all blockers, rather than solving

@pdrhsm
Copy link
Contributor

pdrhsm commented Jul 3, 2017

Removed SerializationInfo and updated to newer version of JetBrain.Annotations here.

I was unable to push my commits to GitTools.Core@feature/netstandard.

Edit: So it seems that all thats left now is waiting for LibGit2Sharp to release a nuget with netstandard support.

@conniey
Copy link

conniey commented Jul 3, 2017

@pdrhsm As a workaround for LibGit2Sharp not having a Portable library... to continue working, in my GitTools.Testing PR, I modified the csproj to conditionally include the portable library version depending on the TargetFramework. GitTools.Testing.Tests.csproj#L32-L40

Also, you should open a PR against feature/netstandard if you want to merge your changes in.

@dazinator
Copy link
Member Author

@pdrhsm I have submitted a PR on your behalf and merged those changes in, thank you.
@conniey - good idea, I'll use the same approach for now.

@dazinator
Copy link
Member Author

Now that GitTools.Core is targeting netstandard1.3 - The GitTools.Core.Tests project which is net45 can't reference it. I'm going to re-target the tests project to net46 to resolve this..

@dazinator
Copy link
Member Author

The next blockers are all with ProcessHelper - it uses ProcessStartInfo, Process and it's kin. These are all not available until NETStandard2.0 - https://github.com/dotnet/corefx/issues/12797

The sole usage for the ProcessHelper within GitTools.Core is for the DumpGraph() method - because it shells out to git.

@dazinator
Copy link
Member Author

dazinator commented Jul 3, 2017

Ok I think this can be reviewed now. I ended up removing DumpGraph on netstandard - because of the lack of system.diagnostic.process api's.

@dazinator
Copy link
Member Author

PR Submitted.

@dazinator
Copy link
Member Author

Waiting on GitTools/GitTools.Testing#6 to be merged, so can then get the tests on travis to run under netcoreapp.

@pdrhsm
Copy link
Contributor

pdrhsm commented Aug 4, 2017

@dazinator as far as I can see this seems to be done?

If so; when can we expect as release?! 👍

@dazinator
Copy link
Member Author

@pdrhsm - GitTools.Core was released to nuget: https://www.nuget.org/packages/GitTools.Core/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants