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

Remove dependency on logula. #161

Merged
merged 1 commit into from Jun 5, 2012

Conversation

Projects
None yet
2 participants
@martiell
Collaborator

martiell commented Jun 3, 2012

Hi Eugene,

What are your thoughts on removing the logula dependency?
I created a pull request so you can see what's involved.

My thoughts are:

  • the extra repository causes problems for people using repository managers
  • there's been no response to my request to publish logula to oss.sonatype.org
  • the diff stat to replace it isn't huge: 89 additions and 55 deletions, and >20 of the additions is license boilerplate. To me, it hardly seems worth the extra dependency for that.

The current logging doesn't seem very clean anyway: logula sets up appenders, only to have the maven plugin remove them, and replace with something else. It seems better if the sbt/maven/cli plugin set up logging, rather than the module.

While I was preparing this patch, I noticed that the integration tests seem to run without logging configured at first. I was running it by using project integration; test in sbt. It seems that logging for these tests only gets configured when GeneralTest runs. I haven't changed that in this commit.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jun 3, 2012

Owner

So this is effectively re-implementing our own logula?

Owner

eed3si9n commented Jun 3, 2012

So this is effectively re-implementing our own logula?

@martiell

This comment has been minimized.

Show comment
Hide comment
@martiell

martiell Jun 4, 2012

Collaborator

Erm. Yes and no.

The parts of logula we're actually using are the Log#forName method (which simply delegates to Logger.getLogger()); some code to set up a console appender; and a Formatter class. The parts we need can be implemented in about 20 lines. So yes in that sense.

(The Log class in the patch is a bit bigger than that because I've moved log configuration out of Module.scala).

No in the sense that the patch doesn't implement another wrapper on top of log4j. Instead logging calls go directly to the log4j logger. Right now, scalaxb isn't using any of the message formatting stuff that logula provides: instead it formats its own logging statements.

There is a middle-ground which would be using logula for logging, but not for log4j configuration. That wouldn't solve the third-party repository problem, but it would avoid some complexity in the maven plugin (and avoid using a superfluous AsyncAppender).

Collaborator

martiell commented Jun 4, 2012

Erm. Yes and no.

The parts of logula we're actually using are the Log#forName method (which simply delegates to Logger.getLogger()); some code to set up a console appender; and a Formatter class. The parts we need can be implemented in about 20 lines. So yes in that sense.

(The Log class in the patch is a bit bigger than that because I've moved log configuration out of Module.scala).

No in the sense that the patch doesn't implement another wrapper on top of log4j. Instead logging calls go directly to the log4j logger. Right now, scalaxb isn't using any of the message formatting stuff that logula provides: instead it formats its own logging statements.

There is a middle-ground which would be using logula for logging, but not for log4j configuration. That wouldn't solve the third-party repository problem, but it would avoid some complexity in the maven plugin (and avoid using a superfluous AsyncAppender).

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jun 4, 2012

Owner

There is a middle-ground which would be using logula for logging, but not for log4j configuration. That wouldn't solve the third-party repository problem, but it would avoid some complexity in the maven plugin (and avoid using a superfluous AsyncAppender).

I am not necessarily against re-implementing logula. I just wanted to understand what we're getting ourselves into.
As long as we don't have the "classpath as configuration" approach of slf4j, I am mostly ok with anything that logs.

Owner

eed3si9n commented Jun 4, 2012

There is a middle-ground which would be using logula for logging, but not for log4j configuration. That wouldn't solve the third-party repository problem, but it would avoid some complexity in the maven plugin (and avoid using a superfluous AsyncAppender).

I am not necessarily against re-implementing logula. I just wanted to understand what we're getting ourselves into.
As long as we don't have the "classpath as configuration" approach of slf4j, I am mostly ok with anything that logs.

@martiell

This comment has been minimized.

Show comment
Hide comment
@martiell

martiell Jun 4, 2012

Collaborator

I saw that you had problems with slf4j, but only on twitter, so I never understood the exact problem you were having.

Do you mean that you want to programmatically configure appenders and logging levels, rather than a config file on the classpath?

Or do you mean you want to programmatically configure which logging backend to use, rather than having the classpath determine it?

The first is possible with slf4j. I doubt it's possible with the second.

Collaborator

martiell commented Jun 4, 2012

I saw that you had problems with slf4j, but only on twitter, so I never understood the exact problem you were having.

Do you mean that you want to programmatically configure appenders and logging levels, rather than a config file on the classpath?

Or do you mean you want to programmatically configure which logging backend to use, rather than having the classpath determine it?

The first is possible with slf4j. I doubt it's possible with the second.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jun 4, 2012

Owner

On both counts, but mostly the latter. The logger got messed up whenever it was executed on modified class loader like specs and sbt. Basic things like changing the log level also couldn't be done programmatically without going down to the underlying logger, rendering the point of facade moot.

Owner

eed3si9n commented Jun 4, 2012

On both counts, but mostly the latter. The logger got messed up whenever it was executed on modified class loader like specs and sbt. Basic things like changing the log level also couldn't be done programmatically without going down to the underlying logger, rendering the point of facade moot.

@martiell

This comment has been minimized.

Show comment
Hide comment
@martiell

martiell Jun 5, 2012

Collaborator

One thing facades like logula and slf4j do provide is conditional formatting, which log4j (on its own) doesn't. For example:

logger.debug("processContext: added sub group %s", sub) // logula
logger.debug("processContext: added sub group {}", sub) // slf4j

rather than:

logger.debug("processContext: added sub group " + sub) // log4j

For scalaxb, the logging can be configured by the cli and build plugins, meaning most of scalaxb can just assume it's configured.

I went back to the commit before the change to logula, and added a logback-test.xml to the integration test project:
https://github.com/martiell/scalaxb/compare/topic/logback

When I did that, the integration tests ran under sbt with debug logging. Other than also making it programatically configurable (for verbose output), what else would need to work? Or rather… how do you run the tests? :)

Collaborator

martiell commented Jun 5, 2012

One thing facades like logula and slf4j do provide is conditional formatting, which log4j (on its own) doesn't. For example:

logger.debug("processContext: added sub group %s", sub) // logula
logger.debug("processContext: added sub group {}", sub) // slf4j

rather than:

logger.debug("processContext: added sub group " + sub) // log4j

For scalaxb, the logging can be configured by the cli and build plugins, meaning most of scalaxb can just assume it's configured.

I went back to the commit before the change to logula, and added a logback-test.xml to the integration test project:
https://github.com/martiell/scalaxb/compare/topic/logback

When I did that, the integration tests ran under sbt with debug logging. Other than also making it programatically configurable (for verbose output), what else would need to work? Or rather… how do you run the tests? :)

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Jun 5, 2012

Owner

For try/dogfood branch, which is the next gen of scalaxb I've been doing more normal specs test. I think even integration test had issues with slf4j. In any case, there's not much we're losing here so let's merge this up.

Owner

eed3si9n commented Jun 5, 2012

For try/dogfood branch, which is the next gen of scalaxb I've been doing more normal specs test. I think even integration test had issues with slf4j. In any case, there's not much we're losing here so let's merge this up.

eed3si9n added a commit that referenced this pull request Jun 5, 2012

@eed3si9n eed3si9n merged commit be9ecc0 into eed3si9n:master Jun 5, 2012

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