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

#230 add in TFM's so dependencies can be optimised #231

Conversation

thompson-tomo
Copy link
Contributor

Add in net coreapp 3.1 & net 6 as TFM to a number of projects so that conditions can be placed on the dependencies to removed explicit dependencies on the newer frameworks

Closes #230

@jodydonetti jodydonetti self-assigned this Apr 20, 2024
@jodydonetti jodydonetti added the enhancement New feature or request label Apr 20, 2024
@jodydonetti jodydonetti added this to the v1.0.1 milestone Apr 20, 2024
@jodydonetti
Copy link
Collaborator

I added some comments per-file, but other than that I get the spirit of your PR and I agree: less explicit dependencies for TFMs that do not require them, great!

In case you agree about the changes, let me know if you want to make them yourself or I can do them.

Thanks!

@thompson-tomo
Copy link
Contributor Author

Happy to make changes @jodydonetti but atm I can't see any proposed changes. Is there a chance you are yet to click finish on your review which publishes the comments?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the TFM should be net7 and not net6, I think, since MemoryPack targets explicitely net7 for some optimizations, see here.

Makes sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why targeting net3.1 explicitly since it's not supported anymore?
Honest question, I've never multi targeted like this before so I'm trying to better understand the TFMs selection strategy here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other comment, selecting net core 3.1 enables that framework and higher to benefit of not adding the dependency. Net 6 was for consistent but we could remove it if desired.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking right now at this as an inspiration and to double check.

There too I can see net6.0 in the <TargetFrameworks> element, even though below there are no conditions targeting that... I'm trying to wrap my head around this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing as above: why targeting net3.1 explicitly since it's not supported anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding net core 3.1 enables net 5 projects to also not have the dependency. Net 6 is to keep it consistent with the other location but could remove it as well,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above about net3.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason i did the 3 frameworks was that System.Threading.Tasks.Extensions was added in netcoreapp 3.1 however System.Diagnostics.DiagnosticSource was only added in net 6.

Copy link
Collaborator

@jodydonetti jodydonetti Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, but if System.Diagnostics.DiagnosticSource was only added in net 6, why its condition is "'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'netcoreapp3.1'"?
I would've expected it has "'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'net6.0'", or am I missing something?

Again, be patient, first time dealing with these kinds of issues 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, the reason for the condition being like this is so that it only adds the dependency when it wasn't a part of the framework. If we were to add net6.0 then it would be included.

@jodydonetti
Copy link
Collaborator

Happy to make changes @jodydonetti but atm I can't see any proposed changes. Is there a chance you are yet to click finish on your review which publishes the comments?

Yep, I forgot to submit the review, sorry about that 🤦

@thompson-tomo
Copy link
Contributor Author

thompson-tomo commented Apr 20, 2024

No problem. Review Comments have just been published so will look at them shortly & come back/make changes.

Copy link
Collaborator

@jodydonetti jodydonetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I think I now get it all, so it LGTM.

I'll merge this and then play a little bit more with it before pushing it out with v1.1.0.

Thanks!

@jodydonetti jodydonetti merged commit 85d599b into ZiggyCreatures:main Apr 21, 2024
1 check failed
@thompson-tomo thompson-tomo deleted the chore/#230_AddTFMToOptimisedDependencies branch April 21, 2024 20:13
jodydonetti added a commit that referenced this pull request Apr 23, 2024
@jodydonetti jodydonetti removed this from the v1.1.0 milestone Apr 23, 2024
@jodydonetti jodydonetti added this to the v1.3.0 milestone Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Optimise dependencies by adding TFM
2 participants