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

Remove dependency on deprecated packages #377

Closed
forki opened this issue Sep 30, 2019 · 33 comments

Comments

@forki
Copy link
Contributor

@forki forki commented Sep 30, 2019

https://docs.microsoft.com/en-us/aspnet/core/migration/22-to-30?view=aspnetcore-3.0&tabs=visual-studio#remove-obsolete-package-references

Giraffe (4.0)
  FSharp.Core (>= 4.7) - restriction: || (>= net461) (>= netstandard2.0)
  Microsoft.AspNetCore.Authentication (>= 2.2) - restriction: || (>= net461) (>= netstandard2.0)
  Microsoft.AspNetCore.Authorization (>= 3.0) - restriction: || (>= net461) (>= netstandard2.0)
  Microsoft.AspNetCore.Diagnostics (>= 2.2) - restriction: || (>= net461) (>= netstandard2.0)
  Microsoft.AspNetCore.Hosting.Abstractions (>= 2.2) - restriction: || (>= net461) (>= netstandard2.0)
  Microsoft.AspNetCore.ResponseCaching (>= 2.2) - restriction: || (>= net461) (>= netstandard2.0)
  Microsoft.IO.RecyclableMemoryStream (>= 1.2.2) - restriction: || (>= net461) (>= netstandard2.0)
  Newtonsoft.Json (>= 12.0.2) - restriction: || (>= net461) (>= netstandard2.0)
  System.Text.RegularExpressions (>= 4.3.1) - restriction: || (>= net461) (>= netstandard2.0)
  System.ValueTuple (>= 4.4) - restriction: >= net461
  System.ValueTuple (>= 4.5) - restriction: && (< net461) (>= netstandard2.0)
  System.Xml.XmlSerializer (>= 4.3) - restriction: || (>= net461) (>= netstandard2.0)
  TaskBuilder.fs (>= 2.1) - restriction: || (>= net461) (>= netstandard2.0)
  Utf8Json (>= 1.3.7) - restriction: || (>= net461) (>= netstandard2.0)

so please remove deps to Microsoft.AspNetCore.*

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Sep 30, 2019

That would require us to switch completely over to netcoreapp3.0, dropping netstandard2.0 and net4x, might be the right time to do it.

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Sep 30, 2019

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Sep 30, 2019

@forki WIP here: #378

@dustinmoris

This comment has been minimized.

Copy link
Member

@dustinmoris dustinmoris commented Sep 30, 2019

Maybe we can keep support for net461 and netcoreapp2.0 for just a bit longer as not everyone has immediate time to switch to the latest framework, but otherwise I agree let's always make sure Giraffe is on the latest with all the recommendations and deprecate support for the other frameworks at the latest by the time MS also stops supporting them. For now, I'd suggest to remove these packages through conditions in the .fsproj file and #ifdefs in code where necessary.

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Sep 30, 2019

Agreed, updated: #378

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Sep 30, 2019

ok serious question: is there anyone not using netcore for giraffe?

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Sep 30, 2019

I would bet not a single soul 😆 Here it doesn't cost us anything on top of ns2.0 IMO

@dustinmoris

This comment has been minimized.

Copy link
Member

@dustinmoris dustinmoris commented Sep 30, 2019

@forki I hope not :), but you never know!

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Sep 30, 2019

@maxdeviant

This comment has been minimized.

Copy link

@maxdeviant maxdeviant commented Sep 30, 2019

ok serious question: is there anyone not using netcore for giraffe?

I didn't even realize you could use Giraffe with .NET Framework.

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Sep 30, 2019

netcoreapp 2.1 will be LTS for another 2 years, I think that should be the minimum bar.

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

@NinoFloris it doesn't seem possible to support both frameworks with one giraffe package.

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Oct 17, 2019

Care to elaborate? @forki

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

for netcore 2.2 we need to reference those packages from giraffe.
for netcore 3 we should not because they no longer exist for that version

so we could in theory add different dependency groups for netcoreapp3 and nectoreapp2.2 in the nuspec of Giraffe. But how does this work with being netstandard2.0?

It's pretty much fucked up.

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

/cc @Krzysztof-Cieslak since the same applies to Saturn

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Oct 17, 2019

It's not great I'll admit, but do you have any particular objection to this:
https://github.com/giraffe-fsharp/Giraffe/blob/develop/src/Giraffe/Giraffe.fsproj#L46-L56

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Oct 17, 2019

As I see it it's pretty 'simple' netcoreapp3.0 has precendence over netstandard2.0.

Hence netstandard should just references all the packages like it did, and netcoreapp3.0 should only have a framework reference + supplemental extension packages etc.

That's exactly how it's setup right now.

EDIT:
Downstream packages take the TFM deps from giraffe for the TFM they're building for afaik, that should all work (for newestget at least).

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

That's not completely correct. There are compiled references into concrete dlls. The platform resolver needs to find the exact corresponding dlls. And it's up to luck that this works.

That said: we should release a version with the proposed changes so that we can try out if it works. Also saturn will need it

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

My assumption is that the proposed change will just work as long as the sdk ships dlls witj same name and same signatures of the stuff that we use

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Oct 17, 2019

What's not completely correct? It's a simplification of all the involved parts but I believe it's not wrong.

The fact you can have restore resolving different concrete dll deps for Giraffe that are not actually referenced in the concrete Giraffe dll for that TFM is an implementation detail and a bug if it goes wrong no?

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Oct 17, 2019

That's why we multi target though.

And to continue my previous comment, multi targetting would be entirely useless if restore didn't at least try to keep restore deps (which ends up in publish) vs concrete dll deps mismatches from happening.

Now the fact FrameworkReferences aren't actually participating in version unification (in nuget) is a whole other fun part and I consider it very bad. It means that packages you reference in your project that aren't already recompiled for netcoreapp3.0 break at runtime when you have a FrameworkReference in your proj (courtesy of web sdk). As it won't warn you during restore that FrameworkReference Microsoft.AspnetCore.App (shipped with sdk 3.0) is not compatible with package that references Microsoft.AspnetCore.App 2.2 (the package on nuget). That part is garbage...

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Oct 17, 2019

What should be changed? @slang25 your changes went in during 4.0 right? @forki does 4.0 work with newestget on core 3.0?

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Oct 17, 2019

No, we need to do another release to satisfy @forki concerns.

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 17, 2019

@NinoFloris the current released package unconditionally references 2.2 stuff.

@NinoFloris

This comment has been minimized.

Copy link
Member

@NinoFloris NinoFloris commented Oct 17, 2019

Ah so #378 just didn't make it. @dustinmoris mind releasing a patch with #378 in there?
I think we should also deprecate the 4.0.0 package afterwards, it'll never work properly on 3.0 without the conditional refs.

@slang25

This comment has been minimized.

Copy link
Member

@slang25 slang25 commented Oct 18, 2019

Looks like @dustinmoris has done a new release:
https://github.com/giraffe-fsharp/Giraffe/releases/tag/v4.0.1

Thanks Dustin 👍

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 18, 2019

image

looks like it's working. Thanks

@forki forki closed this Oct 18, 2019
@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 18, 2019

Corresponding TechEmpower PR: TechEmpower/FrameworkBenchmarks#5162

@atlemann

This comment has been minimized.

Copy link

@atlemann atlemann commented Oct 19, 2019

Is it possible to do these conditional package references with Paket?

@forki

This comment has been minimized.

Copy link
Contributor Author

@forki forki commented Oct 19, 2019

If you do manual template files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.