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

Tmds.DBus #10605

Closed
tmds opened this issue Mar 8, 2023 · 9 comments · Fixed by #10648
Closed

Tmds.DBus #10605

tmds opened this issue Mar 8, 2023 · 9 comments · Fixed by #10648
Labels

Comments

@tmds
Copy link
Contributor

tmds commented Mar 8, 2023

hi, I'm the maintainer of Tmds.DBus.

I noticed you've cloned the library: https://github.com/AvaloniaUI/Tmds.DBus.
And from the changes I wonder if you're forking the code?

If you're forking the code, I wonder if there are changes we can make upstream, which make the fork unnecessary?

cc @kekekeks

@tmds tmds added the bug label Mar 8, 2023
@timunie
Copy link
Contributor

timunie commented Mar 8, 2023

I think it was done in #9810 probably. Let me add @affederaffe as he was the author of the PR.

@kekekeks
Copy link
Member

kekekeks commented Mar 9, 2023

@tmds We are concerned about Nerdbank.Streams dependency pulling way too many additional libraries. This dependency wasn't there previously in Tmds.DBus package. As a UI toolkit, we have to keep the list of dependencies minimal, we've even removed a dependency on System.Reactive because it pulls extra deps when targeting net6-windows10.

Also, we don't see why a DBus library needs System.Security.Principal.Windows dependency.

In the fork I've removed dependencies on those: https://github.com/AvaloniaUI/Tmds.DBus/commit/c6900b5b13c56a6e36462030dbeaca5865e055e8

I doubt that such changes would be accepted to the upstream, so the current plan is to include Tmds.DBus.Protocol as source code with public types made internal.

@tmds
Copy link
Contributor Author

tmds commented Mar 9, 2023

We are concerned about Nerdbank.Streams dependency pulling way too many additional libraries. This dependency wasn't there previously in Tmds.DBus package. As a UI toolkit, we have to keep the list of dependencies minimal, we've even removed a dependency on System.Reactive because it pulls extra deps when targeting net6-windows10.

I will take a PR that removes these dependencies.

Also, we don't see why a DBus library needs System.Security.Principal.Windows dependency.

I'll take a look into this one.

@maxkatz6 maxkatz6 added question and removed bug labels Mar 9, 2023
@tmds
Copy link
Contributor Author

tmds commented Mar 9, 2023

Also, we don't see why a DBus library needs System.Security.Principal.Windows dependency.

This is fixed by tmds/Tmds.DBus#186.

@maxkatz6
Copy link
Member

maxkatz6 commented Mar 9, 2023

@tmds I have a recommendation. Instead of RuntimeInformation.IsOSPlatform(OSPlatform.Windows) it's better to use OperatingSystem.IsWindows() (available starting net6).
This way, .NET compiler will be able to trim this whole dependency (which is still referenced but implicitly now) on non-windows runtime identifiers.

@tmds
Copy link
Contributor Author

tmds commented Mar 9, 2023

Thanks for the recommendation, I didn't know that. I've adopted it here: tmds/Tmds.DBus#187.

@tmds
Copy link
Contributor Author

tmds commented Mar 12, 2023

@kekekeks can you make a PR with your change that removes the dependency on Nerdbank.Streams and its dependencies?

@maxkatz6
Copy link
Member

@tmds done tmds/Tmds.DBus#189

@tmds
Copy link
Contributor Author

tmds commented Mar 13, 2023

The desired changes are now available on the 0.14.0 version which I've just uploaded to nuget.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants