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

Don't include log4j.properties in published JAR #1300

Closed
ryan-williams opened this Issue Dec 3, 2016 · 6 comments

Comments

Projects
3 participants
@ryan-williams
Member

ryan-williams commented Dec 3, 2016

adam-core (cf. 0.20.3) and utils-misc (cf. 0.2.10; and possibly others?) include (identical) log4j.properties in their JARs, leading to some unnecessary logic downstream when assembling uber-JARs that include both.

Specifically, I have to choose to drop one or the other when building an assembly jar in SBT:

assemblyMergeStrategy in assembly := {
  // Two org.bdgenomics deps include the same log4j.properties.
  case PathList("log4j.properties") => MergeStrategy.first
  case x => (assemblyMergeStrategy in assembly).value(x)
}

cf. guacamole, filter-bam, pageant.

I'm not sure what similar downstream Maven-configs do in the presence of multiple files like this, but it seems like logging configuration, in general, should not be included in packaged JARs? I've also recently seen chatter about issues around this in Hadoop-land, e.g. HADOOP-7468.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 3, 2016

Member

Yeah, agreed. Perhaps the best compromise between not spamming logs by default and not pushing properties files would be to put the log4j.properties in the:

  • test resources
  • adam-assembly jar

We push adam-assembly to Maven, but it isn't intended to be used for downstream development, so I think that'd be OK. Thoughts?

Member

fnothaft commented Dec 3, 2016

Yeah, agreed. Perhaps the best compromise between not spamming logs by default and not pushing properties files would be to put the log4j.properties in the:

  • test resources
  • adam-assembly jar

We push adam-assembly to Maven, but it isn't intended to be used for downstream development, so I think that'd be OK. Thoughts?

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Dec 4, 2016

Member

Cool, this isn't something I've thought much about but I guess a rule like "executable/assembly JARs should come with relevant configs while library JARs should not" makes sense.

OTOH I could kind of see a world where even libraries shipped with some default logging config that only set default levels for their own classes/packages, and the contract was that downstream assemblers should have configuration that just appends all log4j.properties files.. I could put that into my parent project configuration in a non-BDG-dep-specific way and be done with it.

Your call / not urgent either way.

Member

ryan-williams commented Dec 4, 2016

Cool, this isn't something I've thought much about but I guess a rule like "executable/assembly JARs should come with relevant configs while library JARs should not" makes sense.

OTOH I could kind of see a world where even libraries shipped with some default logging config that only set default levels for their own classes/packages, and the contract was that downstream assemblers should have configuration that just appends all log4j.properties files.. I could put that into my parent project configuration in a non-BDG-dep-specific way and be done with it.

Your call / not urgent either way.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 4, 2016

Member

Nice idea! I think that's a good approach. Let's get 0.21.0 out first, and then we can get that change done.

Member

fnothaft commented Dec 4, 2016

Nice idea! I think that's a good approach. Let's get 0.21.0 out first, and then we can get that change done.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Dec 10, 2016

Member

I just noticed that I think this is causing one of my downstream libraries' tests to write out adam.log files, even though it's not doing anything ADAM-related, due to this line.

Not a big deal, just noting; I'd never known where that file was coming from.

Member

ryan-williams commented Dec 10, 2016

I just noticed that I think this is causing one of my downstream libraries' tests to write out adam.log files, even though it's not doing anything ADAM-related, due to this line.

Not a big deal, just noting; I'd never known where that file was coming from.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 3, 2017

Member

@heuermh is pretty sure this will be fixed by #1360.

Member

fnothaft commented Mar 3, 2017

@heuermh is pretty sure this will be fixed by #1360.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Apr 6, 2017

Member

Resolved by #1360.

Member

fnothaft commented Apr 6, 2017

Resolved by #1360.

@fnothaft fnothaft closed this Apr 6, 2017

@heuermh heuermh added this to the 0.23.0 milestone Dec 7, 2017

@heuermh heuermh added this to Completed in Release 0.23.0 Jan 4, 2018

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