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

#149 optimise dependencies #152

Merged

Conversation

thompson-tomo
Copy link
Contributor

Closes #149

@AndreaCuneo
Copy link
Contributor

The Description doesn't match the code changes regarding STJ.
In general no reduction in deps is achieved.

@thompson-tomo
Copy link
Contributor Author

@AndreaCuneo there is a reduction in dependencies for net 6 because of the fact that these dependencies are available as part of the framework.

@AndreaCuneo
Copy link
Contributor

@thompson-tomo thanks for the clarification. The description still points to STJ which these changes are unrelated to.

Checking https://nuget.info/packages/Microsoft.Bcl.AsyncInterfaces/8.0.0 you can see that the dependency adds no libs for those targets thus no harm nor security risks: the framework libs are used.

Although reducing deps is good, thanks for the contribution, it adds overhead in management: the VS/Dotnet package manager doesn't handle the conditions thus upgrading packages from tools becomes an hurdle.

Check the conditional dependency on Oracle SqlClient: that has to be handled manually as if you try to upgrade it from any tools, it breaks.

Microsoft provides updates to Microsoft.Bcl.AsyncInterfaces aligned with releases even if it actually has no libs for those targets.

Thus I'm not going to accept this since the reduction is only 'virtual', having the dependency doesn't cause any harm, and adding conditions adds maintenance overhead.

If you have a real case scenario the current dependency create issues, please describe it.

@thompson-tomo
Copy link
Contributor Author

thompson-tomo commented Apr 27, 2024

@AndreaCuneo if you look at the entirity of the change you will see the following changes and reductions:

  • System.Text.Json is only included for net standard 2.0/2.1 & net framework 472 Ark.Tools Ark.Tools.SystemTextJson
  • System.Diagnostics.DiagnosticSource is only included for net standard 2.0/2.1 & net framework 472 and not the net 6 package for Ark.Tools.SystemTextJson and Ark.Tools.NLog
  • Microsoft.Bcl.AsyncInterfaces is only included for net standard 2.0/2.1 & net framework 472 and not the net 6 package for Ark.Tools.Core

I have run into scenarios where an older versions of System.Text.Json was deployed as part of an application because a third party dependency had brought in an older version rather than relying on the one included in the framework which had a feature i needed.

The security scenario is that, if a vulnerability in STJ is found rather than me simply updating my framework on my machine to get the latest patched vesion, i now need to manually update the dependency & perform a release hence a large lead time especially when you are deploying applications on to machines in the field and not just some cloud managed vm's/containers.

@AndreaCuneo
Copy link
Contributor

@thompson-tomo please rebase on master. I've fixed external contributors CI checks not running due to permissions.

@thompson-tomo
Copy link
Contributor Author

@AndreaCuneo all done. 😄

auto-merge was automatically disabled April 27, 2024 13:57

Head branch was pushed to by a user without write access

@thompson-tomo
Copy link
Contributor Author

Had to rebase it yet again

@AndreaCuneo AndreaCuneo merged commit e48bc7d into ARKlab:master Apr 27, 2024
5 checks passed
@thompson-tomo thompson-tomo deleted the chore/#149_DependenciesTweak branch April 27, 2024 21:57
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.

Eliminate STJ dependency for net 6
2 participants