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

Implemented proper support of netstandard2.0 #63

Merged
merged 6 commits into from
Sep 6, 2020

Conversation

NicholasNoise
Copy link

Changelog:

  • removed nuspec using build-in csproj packing.
  • miltiframework test runner: net462, netcoreapp1.0, netcoreapp2.1, netcoreapp3.1 (for future builds of netstandard2.1 or netcoreapp)
  • split NETSTANDARD1_3 and NETSTANDARD2_0 constant (replaced with NETSTANDARD)
  • netstandard2.0 now supports configuration, adonet and other features.

@NicholasNoise NicholasNoise changed the title Implimented proper support of netstandard2.0 Implemented proper support of netstandard2.0 Sep 2, 2020
@NicholasNoise
Copy link
Author

@rgoers @fluffynuts

@fluffynuts
Copy link
Contributor

fluffynuts commented Sep 2, 2020

@NicholasNoise I really appreciate the PR & the time taken to produce multi-target tests! Good job.

There are two changes I'd like to make -- either we can collaborate (if you allow me to push to your PR branch -- https://stackoverflow.com/questions/56350103/how-do-i-allow-just-1-user-to-push-to-a-branch-in-github) or if @rgoers is happy, we can merge in and I'll sort them out later:

  1. now that there are multi-target tests, npm test should invoke all of them. It's not that difficult to do -- or perhaps it just seems that way because I authored zarro too, so I know how it works. I'd like to check that these tests still run through AppVeyer (reminds me: I have to double-check that there's automatic build & test for this repo: I set up AppVeyer against my own fork originally, before I had write access here), for people who perhaps can't run all platform tests locally (I'm not sure if the vs2019 image there supports netcoreapp 1.0 -- I had to install the old sdk to run tests on my machine)
  2. switching from the .nuspec to using inbuild nupkg building is the right thing to do -- but it breaks the release npm script which builds, tests, packs & zips to prepare all artifacts for release. I can see how to fix that by changing the pipeline a little as a release build produces the .nupkg just fine, but it does need to be sorted out. Basically the pack task can be dropped and the zip task should be updated to move the .nupkg to the build/artifacts location before attempting to zip up binary & source artifacts.

all-in-all, I'm happy with the changes -- would just like to address the above. @rgoers since I'm still Quite New here, I'd appreciate your input on any preference as to how things get done?

@fluffynuts fluffynuts mentioned this pull request Sep 2, 2020
@NicholasNoise
Copy link
Author

NicholasNoise commented Sep 2, 2020

@fluffynuts Thank you for quick response!
I made this PR from non-personal fork, but already requested branch setting change to an admin of organization (should be granted by now)

  1. I am not familiar with definition of log4net's build system and hardly understand how it really works. So better you than me :)
    By default VS2019 doesnt support netcoreapp1.0, but sdk can be downloaded and installed manually (link)
    Also I've dropped net35-client and net40-client as target frameworks, because unable to compile against. Should we bring them back and test somehow or deprecate and drop support (someone uses client profile this year?)?
  2. nupgk output directory maybe customized by PackageOutputPath specified in csproj:
<PackageOutputPath>..\..\build\artifacts</PackageOutputPath>

I should mention that csproj has no support of some nuspec properties: summary was deprecated, owners was also deprecated and should be specified on nuget.org package page.
And generates additional framework-specific references:

    <frameworkAssemblies>
      <frameworkAssembly assemblyName="System.Configuration" targetFramework=".NETFramework2.0, .NETFramework3.5, .NETFramework4.0, .NETFramework4.5" />
      <frameworkAssembly assemblyName="System.Web" targetFramework=".NETFramework2.0, .NETFramework3.5, .NETFramework4.0, .NETFramework4.5" />
    </frameworkAssemblies>

@cremor
Copy link

cremor commented Sep 2, 2020

Also I've dropped net35-client and net40-client as target frameworks, because unable to compile against. Should we bring them back and test somehow or deprecate and drop support (someone uses client profile this year?)?

I'd be very careful about dropping target frameworks. This is a breaking change. All projects using that framework will no longer be able to use the newer versions of log4net. I don't know if log4net follows semantic versioning, but if it does, this should only be done in version 3.x

@NicholasNoise
Copy link
Author

I'd be very careful about dropping target frameworks. This is a breaking change. All projects using that framework will no longer be able to use the newer versions of log4net. I don't know if log4net follows semantic versioning, but if it does, this should only be done in version 3.x

Reflecting my previous experience adding or dropping targets are rarely considered as breaking\major change, minor at best.
But anyway I strongly agree that drop is not necessary, this change has been made out of my local environment.
As a reminder to @fluffynuts: better check content of nupgk before publishing it.

@fluffynuts
Copy link
Contributor

fluffynuts commented Sep 2, 2020

Reflecting my previous experience adding or dropping targets are rarely considered as breaking\major change, minor at best.

it's breaking for people who depend on it; I'd (personally) like to drop client-profile targets, but I'm not ready to yet and I think that doing so would justify a major version increment

As a reminder to @fluffynuts: better check content of nupgk before publishing it.

I did, but I forgot to check on client-profile outputs because I'm a derp. Now that I'm reminded of it, I remember not seeing those outputs and I'll add them back in again.

Yes, it's a PITA to get the environment required to build all targets (I've not only done it more than once, but struggled to find off-premise CI to do so!) -- but AppVeyor can do it, so I guess perhaps the real solution might be to provide a non-CP .csproj to encourage contributions, at least for now, or to accept that (again, at least for now), this is a sticky situation.

It doesn't help that getting the SDK for these environments is such a mission because they are well beyond their support lifetime; again, I'd like to drop support for unsupported frameworks (and that includes netapp1.(0|1)!), but that should come in a slightly less shocking format, I think.

@NicholasNoise
Copy link
Author

NicholasNoise commented Sep 3, 2020

Yes, it's a PITA to get the environment required to build all targets (I've not only done it more than once, but struggled to find off-premise CI to do so!) -- but AppVeyor can do it, so I guess perhaps the real solution might be to provide a non-CP .csproj to encourage contributions, at least for now, or to accept that (again, at least for now), this is a sticky situation.

I'm thinking to add new step of PR checklist: is every target framework in place?

It doesn't help that getting the SDK for these environments is such a mission because they are well beyond their support lifetime; again, I'd like to drop support for unsupported frameworks (and that includes netapp1.(0|1)!), but that should come in a slightly less shocking format, I think.

Yep, I think that keeping project on LTS framework is the only way to keep it alive.
Breaking changes such as drop of previously supported frameworks should be planned, documented and consolidated: for example to drop support of netcoreapp1.0 as runtime we should drop netstandard1.*,

🔧 configure msbuild package build & fix up release scripts
🔧 add utility to download & install dotnet core 1.1 sdk
@fluffynuts
Copy link
Contributor

@NicholasNoise please have a look over the changes I've made to see that you're happy with them.

Also, since I know building for the older targets can be a mission, I've added a helper script to get dotnet core 1.1 & updated the docs to draw attention to them. Please try the install-net-framework-sdk-3.5.ps1 helper to see if that resolves your issue with not being able to build against Client Profile (net35 and net40). I last got my machine building against these targets some time ago, so it would help to have verification that the script I wrote then still works (:

@NicholasNoise
Copy link
Author

@NicholasNoise please have a look over the changes I've made to see that you're happy with them.

Also, since I know building for the older targets can be a mission, I've added a helper script to get dotnet core 1.1 & updated the docs to draw attention to them. Please try the install-net-framework-sdk-3.5.ps1 helper to see if that resolves your issue with not being able to build against Client Profile (net35 and net40). I last got my machine building against these targets some time ago, so it would help to have verification that the script I wrote then still works (:

Script works perfectly!
First time build had failed, I made so magic:

  • remove net35-client
  • build
  • add net35-client back
  • build
  • profit!

Other changes are fine with me. Thank you!

@fluffynuts fluffynuts merged commit c728a70 into apache:master Sep 6, 2020
@fluffynuts fluffynuts deleted the netstd20-support branch September 6, 2020 15:28
@fluffynuts
Copy link
Contributor

@NicholasNoise btw, I'd just like to update the build system in master to produce the sha512 sigs at build time, sign artifacts from appveyer, update the site and then I'll propose a vote to release 2.0.10 since it improves netstandard2.0 support for all users.

@rgoers
Copy link
Member

rgoers commented Sep 6, 2020

I would suggest the next release include the fix for the open CVE as it is confusing users as they are expecting it. My understanding is the the fix is on the develop branch but was never merged to master.

@fluffynuts
Copy link
Contributor

@rgoers good idea. It's in a commit on develop and I'm not sure what else is in there, so I'd like to simply cherry-pick that commit. I'll do so now.

@NicholasNoise
Copy link
Author

@NicholasNoise btw, I'd just like to update the build system in master to produce the sha512 sigs at build time, sign artifacts from appveyer, update the site and then I'll propose a vote to release 2.0.10 since it improves netstandard2.0 support for all users.

Thanks!
Patiently (or maybe just a little impatiently) waiting the release. ETA?

@fluffynuts
Copy link
Contributor

I've started the vote process, so it's pretty-much up to the rest of the list and how much I've managed to mess up on this release (it's only my second, but the first could have been way better, so 🤞 )
I've also included a fix for a long-standing CVE, so I'm hoping that will make some other people happy too (:

@fluffynuts
Copy link
Contributor

fluffynuts commented Sep 12, 2020

@NicholasNoise the vote passed, package 2.0.10 is up at nuget.org; the site should hopefully be updated when someone with the required permissions to publish has some time.

@NicholasNoise
Copy link
Author

NicholasNoise commented Sep 12, 2020

@fluffynuts thanks!
I'll test it in live environment and come back with a feedback.

btw, this release is a bit messed up (1, 2, #66), but no harm done.

@fluffynuts
Copy link
Contributor

btw, turned out I have the required permissions... still new here 😳 anyhoo, the more we do this, the better we get, right? At least, that's the plan (:

@NicholasNoise
Copy link
Author

the more we do this, the better we get, right? At least, that's the plan (:

This is the way...
If it turns sideways we change plan (:

@cremor
Copy link

cremor commented Sep 15, 2020

btw, this release is a bit messed up (1, 2, #66), but no harm done.

About the wrong versions: I would suggest to set GenerateAssemblyInfo to true in the csproj to let the build create all those assembly attributes. Right now the version numbers are scattered over 3 files (6 if you count that weird cpp/js/vb files) which always has the risk of forgetting one.

In my opinion, the different informational versions and titles for the differnet target frameworks can be removed. You see the target framework in the TargetFrameworkAttribute anyway. But if you want to keep them I'm sure there is a way to do it in a single csproj tag using csproj variables.

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.

4 participants