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

Variuos small changes #42

Closed
wants to merge 6 commits into from
Closed

Variuos small changes #42

wants to merge 6 commits into from

Conversation

HavenDV
Copy link

@HavenDV HavenDV commented Mar 30, 2024

  • DotNet.ReproducibleBuilds is official Microsoft NuGet inside dotnet org: https://github.com/dotnet/reproducible-builds
  • This is required in order to make the assemblies deterministic. You can read more about this, but right now you are missing some CI specific properties:
    image
  • You can check metadata here: https://nuget.info/packages/MyCSharp.HttpUserAgentParser/3.0.1
  • global.json is not needed because it prohibits the use of new versions of the sdk (and, as a result, the latest code analyzers). this will also not work for CICD as the dotnet 8.0.100 sdk will not allow you to run the net7 target framework tests.
  • Nuget.config contains default values and can be removed, the default behavior is exactly the same as described
  • Microsoft.AspNetCore.Http and Microsoft.AspNetCore.Http.Abstractions are deprecated and their use is not recommended. You can check it here - https://www.nuget.org/packages/Microsoft.AspNetCore.Http
    image
  • The replacement for it is to use <FrameworkReference Include="Microsoft.AspNetCore.App" />. But this doesn't make sense in the main library, so HttpContextExtensions.cs has been moved to the AspNet library.
  • net7 has been removed from testing because net7 has a short lifecycle and it expires after 50 days, and many developers no longer have the net7 sdk installed. However, with only the net8 sdk installed it is impossible to run net7 tests

@BenjaminAbt
Copy link
Member

BenjaminAbt commented Mar 30, 2024

Thank you for your PR but...

  • you deleted recommended files for CICD - why?
  • You only added DotNet.ReproducibleBuilds which is not required / wanted in this repo
  • The other changes dont make sense.

Thank you. But I have do close this.

@HavenDV
Copy link
Author

HavenDV commented Mar 31, 2024

@BenjaminAbt Sorry, I missed the explanation block:

  • DotNet.ReproducibleBuilds is official Microsoft NuGet inside dotnet org: https://github.com/dotnet/reproducible-builds
  • This is required in order to make the assemblies deterministic. You can read more about this, but right now you are missing some CI specific properties:
    image
  • You can check metadata here: https://nuget.info/packages/MyCSharp.HttpUserAgentParser/3.0.1
  • global.json is not needed because it prohibits the use of new versions of the sdk (and, as a result, the latest code analyzers). this will also not work for CICD as the dotnet 8.0.100 sdk will not allow you to run the net7 target framework tests.
  • Nuget.config contains default values and can be removed, the default behavior is exactly the same as described
  • Microsoft.AspNetCore.Http and Microsoft.AspNetCore.Http.Abstractions are deprecated and their use is not recommended. You can check it here - https://www.nuget.org/packages/Microsoft.AspNetCore.Http
    image
  • The replacement for it is to use <FrameworkReference Include="Microsoft.AspNetCore.App" />. But this doesn't make sense in the main library, so HttpContextExtensions.cs has been moved to the AspNet library.
  • net7 has been removed from testing because net7 has a short lifecycle and it expires after 50 days, and many developers no longer have the net7 sdk installed. However, with only the net8 sdk installed it is impossible to run net7 tests

@BenjaminAbt
Copy link
Member

BenjaminAbt commented Mar 31, 2024

You are active in the open source scene, so you should know that direct PR without discussion is usually not a good idea :-)

You don't need an extra NuGet package for deterministic builds. There has been an MSBuild parameter for years, and now also a dotnet pack parameter.

We already have this with other NuGet enhancements on our agenda.

many developers no longer have the net7 sdk installed

.NET 7 is still more used than .NET 8 today tho

Nuget.config contains default values and can be removed, the default behavior is exactly the same as described

watch the contents again, they are not default

global.json is not needed because it prohibits the use of new versions of the sdk

thats not how the global.json works tho

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.

2 participants