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

Newest version of Microsoft.Extensions.Logging.Abstractions requires too many dependencies #1106

Closed
gregoryjtom opened this issue Dec 13, 2022 · 14 comments

Comments

@gregoryjtom
Copy link

FluentFTP Version: 42.1.0

Currently, the README states that the library has only one dependency - Microsoft.Extensions.Logging.Abstractions. However, this is only true if you use the 2.1.0 version of that library.

For my use case, I would like to use the most up-to-date version (currently 7.0.0) of the library for security/reliability reasons, but this would require 15+ more dependencies because of added dependencies for the newer versions. A solution to this would be much appreciated. Further, once Microsoft deprecates all older versions of the library, we will all run into this problem at some point.

@robinrodricks
Copy link
Owner

robinrodricks commented Dec 14, 2022

Hi @gregoryjtom , good point but this was a new change in v40 requested by a number of people. See the v40 discussion here. #906

So essentially some users wanted to use standard logging libraries which we have never supported since day 1. v40 was a big change and it seemed like a good time to add that in. But the downside is we have a dependency on MELA which is unavoidable as we need the ILogger interface from it.

What do you suggest for a solution?

One idea: Publish 2 different nuget libraries. One which references MELA and one which does not.

  1. FluentFTPLite will be just the pure library without any deps
  2. FluentFTP will have MELA

(or flip the naming and make FluentFTP the plain one and FluentFTP.AnySuffix the MELA one - where AnySuffix must be decided)

@jnyrup
Copy link
Contributor

jnyrup commented Dec 22, 2022

@gregoryjtom what framework is your application targeting?
I've tried building a console program using VS2022 targeting net462, net48 and net6 and different versions of MELA to see what dlls are included in the bin folder.

  • MELA 2.1.0 & 2.2.0 & 3.1.0
    • net462:
      • 101 dlls
    • net48:
      *Microsoft.Extensions.Logging.Abstractions
    • net6:
      • Microsoft.Extensions.Logging.Abstractions
  • MELA 6.0.0 & 7.0.0:
    • net462 & net48:
      • Microsoft.Extensions.Logging.Abstractions
      • System.Buffers
      • System.Memory
      • System.Numerics.Vectors
      • System.Runtime.CompilerServices.Unsafe
    • net6:
      • Microsoft.Extensions.Logging.Abstractions

Without have done exhaustive experimentation it looks like if you use at least net48 you should not see 15+ dlls.

@robinrodricks
I would not recommend duplicating the core library in two packages.

  • From a consumer perspective it might make some confusion about the differences.
  • It makes it hard if having project dependencies A -> B -> C and some use FluentFtp and some FluentFtpLite.
  • (Your download count on nuget.org will be distributed between two packages, which might make it less visible.)

If you want to go with publishing multiple packages, I proposed in #906 (comment) to make logging using MELA an extension package such that you have

  1. FluentFtp all ftp functionality and built-in logging
  2. Fluent.Ftp.Logging a tiny package to help bridge between the built-in logging and MELA.

@robinrodricks
Copy link
Owner

Hi @jnyrup thanks for the testing and your comments. Yes I agree that we should not duplicate the core packages.

FluentFTP + FluentFTP.Logging sounds like a good possibility if people don't want MELA always bundled.

@FanDjango
Copy link
Collaborator

I think that is a good idea. I have also run into this problem to a certain degree.

@robinrodricks
Copy link
Owner

@jnyrup Any idea how these 2 packages would interact?

  • FluentFTP Exposes some IFluentLogger
  • FluentFTP.Logging implements IFluentLogger and provides an implementation to it which also implements MELA.ILogger?

I'm finding it hard to understand what FluentFTP.Logger really does here, apart from adding a ref to MELA package.

@jnyrup
Copy link
Contributor

jnyrup commented Dec 22, 2022

With help from https://stackoverflow.com/questions/5646820/logger-wrapper-best-practice/5646876#5646876 here's a prototype of how it could be structured.
https://github.com/jnyrup/FluentFTP/commit/5c248df803a6857ff6ff84dbedbc9f5c1b9cc2e0

@robinrodricks
Copy link
Owner

robinrodricks commented Dec 23, 2022

@jnyrup Wow that was fast. I like this implementation overall, especially how it eliminates the forced dep on MELA. How would one use this in practice?

new FtpClient(h, p, c, new MicrosoftLoggingAdapter(new Log4NetLogger())) ??

@robinrodricks
Copy link
Owner

Merged into master. Any suggested next steps @jnyrup or should I release? @FanDjango you want to play with it before release?

@jnyrup
Copy link
Contributor

jnyrup commented Dec 23, 2022

How would one use this in practice?
new FtpClient(h, p, c, new MicrosoftLoggingAdapter(new Log4NetLogger())) ??

yes

Any suggested next steps @jnyrup or should I release?
Did you see the comments in my prototype in the .csproj file about how this should be done properly?

<!-- Should reference the first version of FluentFTP that exposes IFluentFtpLogger -->
<!--<PackageReference Include="FluentFTP" Version="42.2.0" />-->
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.1.0" />
</ItemGroup>
<ItemGroup>
<!-- this is just to make it compile, FluentFTP should be referenced through a <PackageReference> -->
<ProjectReference Include="..\FluentFTP\FluentFTP.csproj" />

@FanDjango
Copy link
Collaborator

@jnyrup @robinrodricks

I've played with it. Yippee... no more MELA to be distributed along with my application - I tested the case where I am not using it.

This is worthy of a release. Needs doc in Wiki of course, this change so soon after V40 introduction of external logging.

@robinrodricks
Copy link
Owner

Yippee... no more MELA to be distributed along with my application - I tested the case where I am not using it.

Great.

Needs doc in Wiki of course, this change so soon after V40 introduction of external logging.

Why not. Most people would not have used MELA anyway.

Did you see the comments in my prototype in the .csproj file about how this should be done properly?

Yes but thanks for the reminder.

@robinrodricks
Copy link
Owner

robinrodricks commented Dec 25, 2022

Published v43. I have wait for Nuget to update then I can publish the first version of FluentFTP.Logging

43.0.0

  • Please read the Migration Guide for help migrating to the new version!
  • Packaging
    • Core FluentFTP package has removed the dependency on MELA (Microsoft.Extensions.Logging.Abstractions)
    • New FluentFTP.Logging package released that integrates with MELA
  • Logging
    • FtpClient.Logger is no longer a MELA ILogger
    • FtpClient.Logger is now a custom interface called IFtpLogger

@robinrodricks
Copy link
Owner

I am closing this issue. If further issues arise with logging, please file a new issue.

Thanks all.

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

No branches or pull requests

4 participants