Removes the slf4j implementation from the classpath when running Cobertura #4198

Merged
merged 2 commits into from Jan 25, 2017

Conversation

Projects
None yet
3 participants
@jebbench
Contributor

jebbench commented Jan 23, 2017

Problem

The Cobertura plugin adds SLF4J to the classpath when generating code coverage; as a result it's not possible to run tests that depend on using their own slf4j implementation.

See #4197

Solution

Removed slf4j-simple from the Cobertura runtime classpath.

Result

There is no slf4j implementation on the classpath when coverage is generate unless an implementation is added by the user.

If Cobertura fails then it's logging output will be directed to the SLF4J NOOP logging implementation (so it will fail silently).

@@ -55,8 +55,7 @@ def cobertura_jar(**kwargs):
register_jvm_tool(register,

This comment has been minimized.

@jsirois

jsirois Jan 23, 2017

Member

Please amend the comment above, maybe ", but not any of its dependencies except an slf4j implementation for logging. We leave this out to trade ..." and reference a new issue that tracks shading cobertura as a more complete solution.'

@jsirois

jsirois Jan 23, 2017

Member

Please amend the comment above, maybe ", but not any of its dependencies except an slf4j implementation for logging. We leave this out to trade ..." and reference a new issue that tracks shading cobertura as a more complete solution.'

This comment has been minimized.

@jebbench

jebbench Jan 24, 2017

Contributor

Done.

@jebbench

jebbench Jan 24, 2017

Contributor

Done.

@jsirois

LGTM, just one more request.

@jsirois

This comment has been minimized.

Show comment
Hide comment
@jsirois

jsirois Jan 24, 2017

Member

LGTM. The CI failure is not your problem, but it blocks merging. Its also affecting a PR of mine (#4201). The issue is tracked by #4204 which #4201 may provide a short-term fix for. As soon as that fix is in master, a rebase here will be needed to pick the fix up and hopefully get this branch green under travis.

Member

jsirois commented Jan 24, 2017

LGTM. The CI failure is not your problem, but it blocks merging. Its also affecting a PR of mine (#4201). The issue is tracked by #4204 which #4201 may provide a short-term fix for. As soon as that fix is in master, a rebase here will be needed to pick the fix up and hopefully get this branch green under travis.

@jsirois

This comment has been minimized.

Show comment
Hide comment
@jsirois

jsirois Jan 25, 2017

Member

@jebbench you should be able to rebase later today when the fix in #4205 lands.

Member

jsirois commented Jan 25, 2017

@jebbench you should be able to rebase later today when the fix in #4205 lands.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jan 25, 2017

Member

@jebbench : The fix from #4205 is now in master. Sorry for the trouble!

Member

stuhood commented Jan 25, 2017

@jebbench : The fix from #4205 is now in master. Sorry for the trouble!

@jebbench

This comment has been minimized.

Show comment
Hide comment
@jebbench

jebbench Jan 25, 2017

Contributor

Rebased off master.

Contributor

jebbench commented Jan 25, 2017

Rebased off master.

@jsirois jsirois merged commit d95447f into pantsbuild:master Jan 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jsirois

This comment has been minimized.

Show comment
Hide comment
@jsirois

jsirois Jan 25, 2017

Member

Thanks @jebbench - this should go out in the 1.3.0.dev9 release this Friday January 27th.

Member

jsirois commented Jan 25, 2017

Thanks @jebbench - this should go out in the 1.3.0.dev9 release this Friday January 27th.

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