Feature/netstandard2 (NETCore 2.1 support) #1422
Conversation
|
The mentioned PR has been merged and a preview of LibGit2Sharp has been published to nuget. When can we expect a merge? |
|
@li0nsar3c00l I can't really say, PRs seem to hang around for really long times in this repo. 😄 The PR this one is based on has been around for almost 11 monts now. Looks like it is finally about to merge though. |
|
@JakeGinnivan @dazinator There was discussion in #1269 about merging in GitTools.Core and I agree with the idea. Do you all want me to do that as part of this PR? It would certainly simply things not having to git the PR in GitTools.Core merged/released first. |
|
@kll I recon. Lets do it, anything to simplify this project for now. The plan was to have a useful separate testing library, but we can release it from here if people want it. I rebased this PR to remove the duplicated commits, it's running for me so if you want to move those two libraries into this PR that is cool |
kll
force-pushed
the
feature/netstandard2
branch
2 times, most recently
from
7153e02 to
5c294e1
Compare
|
@JakeGinnivan Done. I also went ahead and updated to the latest prerelease of LibGit2Sharp so that we could use the latest LibGit2Sharp.NativeBinaries which solves my primary problem of being able to easily use GitVersion from the latest dotnet docker images. I had it referencing a release version of LibGit2Sharp but since GitTools.Core was previously referencing a prerelease version of LibGit2Sharp anyway I decided to continue the trend for now. |
|
@kll thanks for all your effort! Hoping for a fast merge. |
|
@dazinator Imo yes, #1416 and #1248 can both be closed since this pullrequest includes the latest preview version |
|
I agree, this should supersede both of those. |
|
Before we merge and release this to nuget.org, can we ensure we release whats currently in master to nuget.org, as those packages will support pre |
|
@JakeGinnivan what do you think about this? Should we release a 4.0.0 that supports netstandard 1.3 and then another release 4.1.0 that supports 2.0? Does that open up a lot of potential for support issues? @dazinator what are your thoughts here? |
|
@gep13 - That would be my preferred route yes. The PR that targets the lower If we don't release this to nuget.org, I can always continue to consume the appveyor ci nuget feed to obtain the PR packages, as long as those packages are retained. However I would prefer a proper release to nuget.org for peace of mind because I am not sure what the retention rules are in appveyor for packages and pr branches. Once this PR is merged, we should just say we no longer support the older version, but atleast it's available for those that need it like me :-) |
kll
force-pushed
the
feature/netstandard2
branch
from
232f02c to
0d4adac
Compare
|
To ensure compatibility with higher sdk's, I'd recommend the removal of the |
|
@dazinator isn't part of the reason .net core is good is you can side by side. Why can't we land this and then external projects still be on 1.1? (honest question) |
|
@JakeGinnivan |
| @@ -9,7 +9,7 @@ | |||
| <OutputType>Library</OutputType> | |||
| <RootNamespace>GitVersionTask</RootNamespace> | |||
| <AssemblyName>GitVersionTask</AssemblyName> | |||
| <TargetFrameworks>net461;netstandard1.5</TargetFrameworks> | |||
| <TargetFrameworks>net461;netstandard2.0</TargetFrameworks> | |||
There was a problem hiding this comment.
Can net461 now be removed here, as net461 implements netstandard20?
There was a problem hiding this comment.
.. actually thinking about this further I think there was a legitimate reason to leave this, perhaps something to do with util pack restore. I can't precisely remember the reason now, but safest to leave for now if it's not causing any harm
There was a problem hiding this comment.
Yeah I wasn't really sure if it was OK to remove it in the task project or not because I didn't have any real experience with it so I decided it was safer to leave it alone until I was told otherwise. :)
There was a problem hiding this comment.
As far as I know, that way it can be referenced from other .NET Standard libs and doesn't cause problems with an implicit reference to the full .NET Framework
There was a problem hiding this comment.
@li0nsar3c00l That isn't how referencing multi-targeted nuget packages works. The project system will do the right thing and only pull in the most applicable version of the referenced library. If you reference it from a netstandard2 project you will only end up with a reference to the netstandard2 version of the library.
|
@li0nsar3c00l
I think it would be good to get a release of master out the door for reasons mentioned above, and also, at the moment I don't see any evidence that we have a handle on releasing. It would be good to get our house in order on the releasing front before we accept any future PR's however, we could merge this first and then sort the release out after, I don't mind. @JakeGinnivan @gep13 what say you? |
|
@kll Any thoughts on my recent comment? Do you agree, that this should be fixed? In case you do, should I create a pull request against your repo or do you want to edit that on your own? |
|
Anybody PRESS MERGE BUTTON!!! ) |
|
@petriashev merging this won't do much good until we also sort #1445 - because merging wont result in any release at present. |
|
I am not sure whether it's intended or not, but when |
|
Does appveyor have a nuget repository I can access to already start using this? This seems empty: Is there a possibility people forget #1445 for a while and release a netstandard2 version manually, so that everybody can continue to enjoy this? It is nice and all that you want the #1445 sorted out, but you have consumers that would like to move forward. Maybe this repository has too many build artifacts. Does it make sense to split the artifacts out over different repositories that can use this repository as an upstream source? Hereby making it possible to actually let some of the downstream artifacts die if no one actually continues to contribute to them. |
So many 👍 ! I've been lurking at this PR since months and can't hide my excitement to see this now almost being done and merged. Is there anything I could help with to get this merged faster? Is there an easy consumable nuget feed (e.g. at appveyor) where I could get the nuget from and see if this works in my build process for example? (I've had a quick look but didn't find anything) Thanks for the good work! |
|
Is there anything the community can do to help? This has downstream effects, like not being able to use GitVersion on .NET Core/Ubuntu 18. |
|
I'm not aware of anything holding this up. It just seems to sit here. Pretty soon I'll need to abandon the PR because I no longer have the time to maintain it and fix all the conflicts coming from master. This needs to get merged ASAP. |
|
There is one issue we have to implement #1493, as at the moment the build scripts is not creating a release in GitHub and no release notes are generated, and there is no stable version created when we set a release tag. I started working on that, but don't have too much time to finish it. Even if we merge it now, we still have to manually trigger the stable version build. We probably need to increase the minor version to 4.1.0. @dazinator , @gep13 , @JakeGinnivan, @asbjornu what do you think? Meanwhile, I would recommend if possible to squash the commits into 2 commits -
|
|
To me that is beside the point. There is no reason this one particular thing has to wait for the fully fleshed out release process when I see commits landing in master regularly . It is creating extra work for me and I see no valid reason for it. |
Unit test projects have to stay on net461 for now due to some of the libraries they consume. Bring GitTools.Core and GitTools.Testing directly into this repository. Update LibGit2Sharp and LibGit2Sharp.NativeBinaries Set new build scripts to use net461 Consolidate versions of some unit testing related nuget packages.
kll
force-pushed
the
feature/netstandard2
branch
from
980591d to
031834e
Compare
|
As for version numbers... I'd argue that updating to a higher major version of frameworks and SDKs qualifies as a breaking change. If we follow SemVer (which is what this repo is kinda all about) then it probably needs to be 5.0 rather than 4.1. |
|
I agree on the breaking change semver major --> 5 as this will drop support for some lower target platforms --> so it is a breaking change. That raises the question, what is the best way for us to bump the version in this case? Should we:-
I think the bulk of what we needed to do is done, thanks a lot to your work on the CI / CD process @arturcic . We can now release stuff to all the places, even if we have to kick of the process manually, it;s so much better than before that we actually can turn PR's arround now, by getting everything distributed to everywhere with minimal fuss. We can continue to tweak / improve the process with the release notes etc, but I don't think this should block PR's being merged any longer. Saying that, this PR does have quite a lot of changes to review, due to the conflation of some other issues. |
|
If we set a tag on current master as |
|
Yep, we can tag as |
|
Let's merge it! |
|
@arturcic: Done! Let's see how this affects the build. |
|
Great!!!, I'll check as well how the build goes |
|
It's version 4.0.1-beta.1+53, so I suggest to wait for the build and see everything builds and then we can add 5.0.0-alpha1 as tag and do a manual build so that we get the version changed |
|
@arturcic: Seems like the builds succeeded: ✅ Azure DevOps Unless anyone objects, I'll tag cf2ccfa with |
|
@arturcic: Aha, thanks for pointing that out. That needs to be fixed, yes. We should probably also tag as |
Agree. Right now checking why it's not included LibGit2Sharp, will update it here as well when I have fixed so that you can proceed with the tag |
|
@arturcic: Thanks for looking into it! 🙏 |
Changing the target to netstandard2.0 allows upgrading to the latested versions of LibGit2Sharp and its associated LibGit2Sharp.NativeBinaries packages.
This allows the dotnetcore version to run on all the .NET Core 2.1 supported OS versions.
I left the unit tests targeting the full desktop framework but bumped them to net461 so they could consume the netstandard2.0 GitVersionCore project. The reason I didn't convert everything over is that there are several nuget packages used in the test projects that haven't been updated in a long time and do not have any netstandard support at all.