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

FitNesse declares a compile-time slf4j implementation dependency #1248

Closed
briehman opened this issue Nov 26, 2019 · 8 comments
Closed

FitNesse declares a compile-time slf4j implementation dependency #1248

briehman opened this issue Nov 26, 2019 · 8 comments

Comments

@briehman
Copy link
Contributor

The slf4j-jdk14 dependency is listed as a compile dependency within FitNesse. This causes it to be seen as a transitive dependency for anybody using FitNesse unless they explicitly exclude it. This can cause conflicts with their existing slf4j implementations, as it did with mine, since they may not be expecting a library to be including an slf4j implementation as a dependency.

The [slf4j user manual|http://www.slf4j.org/manual.html] encourages libraries to only utilize the interface and not the implementation:
"Authors of widely-distributed components and libraries may code against the SLF4J interface in order to avoid imposing an logging framework on their end-user. Thus, the end-user may choose the desired logging framework at deployment time by inserting the corresponding slf4j binding on the classpath, which may be changed later by replacing an existing binding with another on the class path and restarting the application. This approach has proven to be simple and very robust. "

@fhoeben
Copy link
Collaborator

fhoeben commented Nov 26, 2019

You are absolutely right. But FitNesse is not (only) a library, it is also an application (for which including the logging implementation is normal).
Users of the standalone version should get logging, but uses that use it as a library should only have the API (and no implementation, although a default is good for backwards compatibility for people who are not using slfj4 right now and should not be required to add an additional library, they never needed in the past, just to upgrade their FitNesse).

Do you see a way we can 'just make it work' for people who are not using slf4j in their own code at the moment, while preventing problems for people who do want to choose their own implementation?

In the past I chose the current approach because I figured: people who are using slf4j and have chosen their implementation are more technically savvy, and will have be able to exclude the one included in FitNesse, and it will work out of the box for people who are using the standalone version and/or just want to get logging to their console and their execution log (and don't really care whether that is done via implementation X or Y).

It's not a great solution, but a choice. I would like to prevent problems for people using their own slf4j implementation, but the only way I see that will not make things harder for other users is documenting more explicitly (on the downloads page?) that an implementation is present and that one should exclude it one prefers to use another.

Do you see a better approach?

@briehman
Copy link
Contributor Author

Hi Fried,

That makes sense indeed for why it is packaged in this way. Unfortunately, the only idea I have as a possible solution is quite intrusive. If the fitnesse artifact was split into two components, fitnesse and fitnesse-standalone, it could support this scenario. The fitnesse artifact could depend upon the standalone version and include any required runtime dependencies in that project such as an slf4j implementation.

This is quite heavy-handed however and I would imagine it is not a trivial amount of work. Additionally, it could potentially be confusing for users as to which published artifact they should use under which scenario.

Indeed, perhaps simply adding additional documentation is a better place to begin as opposed to something more invasive and time-consuming.

fhoeben added a commit to fitnesse/fitnessedotorg that referenced this issue Dec 17, 2019
@fhoeben
Copy link
Collaborator

fhoeben commented Dec 17, 2019

What do you think, is the note added to http://fitnesse.org/FitNesseDownload sufficient?

FitNesse includes ''slf4j-jdk14'', an slf4j implementation, to write its logging. Users who want to use a different logging implementation should exclude ''slf4j-jdk14'' so there will be no conflicts.

@briehman
Copy link
Contributor Author

Yes, I think that will be very helpful. Thank you!

@roboaks
Copy link

roboaks commented Dec 26, 2019

In my situation, I am using fitnesse-standalone and I have a fixture with this Gradle dependency:

implementation 'ch.qos.logback:logback-classic:1.2.3'

I am getting this exception when running a test from FitNesse:

__EXCEPTION__:java.lang.ClassCastException: org.slf4j.impl.JDK14LoggerAdapter cannot be cast to ch.qos.logback.classic.Logger
at com.its.itap.fixtures.selenium.helpers.ItapLogger.<init>(ItapLogger.java:61) [SeleniumFixture-all.jar]
at com.its.itap.fixtures.selenium.helpers.ItapLogger.initialize(ItapLogger.java:127) [SeleniumFixture-all.jar]
at com.its.itap.fixtures.selenium.helpers.FixtureFactory.createForItap(FixtureFactory.java:62) [SeleniumFixture-all.jar]
...

presumably due to the fact that slf4j-jdk14 is on the runtime classpath.

Can you suggest a solution?

@fhoeben
Copy link
Collaborator

fhoeben commented Dec 26, 2019

With the fitnesse standalone version I don't believe there is an easy way to exclude the logging jar.

So you'll either have to switch to a gradle setup for fitnesse (and exclude slf4j-jdk14), or create your own version of the standalone.jar (unpack it, remove the slf4j-jdk14 classes and re-zip it), or ask the developers of the fixture to not include (and not depend on) a specific slf4j implementation (so your tests can just work with the implementation provided by FitNesse).

Like stated in this issue it is not good form for a library to include a slf4j implementation:

Authors of widely-distributed components and libraries may code against the SLF4J interface in order to avoid imposing an logging framework on their end-user. Thus, the end-user may choose the desired logging framework at deployment time by inserting the corresponding slf4j binding on the classpath, which may be changed later by replacing an existing binding with another on the class path and restarting the application. This approach has proven to be simple and very robust.

And while I argued above that FitNesse, and specifically the standalone version, is not a library but an application and should therefore provide an slf4j implementation, a fixture is a library and should not impose one on its users...

@roboaks
Copy link

roboaks commented Dec 26, 2019

Thanks so much for your help on this Fried. I do understand your point that a fixture should not impose a logging imp. The issue is that we have extensive junit tests associated with the fixture that do their own logging--so we need an implementation.

We will adopt one of your two suggestions :)

@woodybrood
Copy link
Collaborator

Seems like this is addressed. Download page was updated and user is going to try workaround.

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

No branches or pull requests

4 participants