Details
-
Bug
-
Status: Closed
-
Trivial
-
Resolution: Fixed
-
3.1.0-incubating
Description
Pull request: https://github.com/apache/incubator-tinkerpop/pull/229
Attachments
Issue Links
Activity
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186244839
Yes, the "app" projects should have the runtime dependencies marked as "scope=runtime", and the "library" projects should have these dependencied marked as "scope=test"
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186249643
Cool - that makes sense - can you please amend your PR appropriately and take all the modules into account in doing so?
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186262127
Also, when you do that, can you please target this change at the `tp31` branch? I think we'd want to see this change for `3.1.2-incubating`.
GitHub user ceefour opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/233
slf4j-log4j12 / log4j is only required for testing (TINKERPOP-1151)
slf4j-log4j12 / log4j is only required for testing (TINKERPOP-1151)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ceefour/incubator-tinkerpop TINKERPOP-1151
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/233.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #233
commit 129f86d61713982aa3c19bc9330f54e4550306f0
Author: Hendy Irawan <hendy@hendyirawan.com>
Date: 2016-02-13T07:21:07Z
slf4j-log4j12 / log4j is only required for testing
commit d7fac2a8dab014e9558e9ba895397379cf78fa53
Author: Hendy Irawan <ceefour666@gmail.com>
Date: 2016-02-19T23:41:46Z
slf4j-log4j12 / log4j is only required for testing (TINKERPOP-1151)
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186455720
Replaced by #233
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-186455947
Okay @spmallette, I hope this is good
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-187275963
did you try to build this? it doesn't build for me or travis/appveyor....
Github user pluradj commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-188827536
build fails for me too, similar to travis
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-192293507
@ceefour do you expect to make any revisions to this to get it to build?
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-196802767
@ceefour anything new here with respect to getting this PR to build? if not, I'd like to close this.
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-196806051
@spmallette Sorry, I'm currently fixing it now.
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-196843308
@spmallette Fixed
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-198128052
I'm not sure if this is right still. The tests run but i still see this message in every module during build:
```text
Running org.apache.tinkerpop.gremlin.util.function.ScriptEngineLambdaTest
Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.648 sec - in org.apache.tinkerpop.gremlin.util.function.ScriptEngineLambdaTest
Running org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutorTest
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
```
I haven't even gotten to testing the actual console distribution, server distributions, hadoop/spark/giraph logging. Can you please post some evidence of these things working in full? without that, i don't think i can VOTE positively on this.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-200452143
Something is still wrong:
```text
$ bin/gremlin.sh
\,,,/
(o o)
----oOOo(3)oOOo----
plugin activated: tinkerpop.server
plugin activated: tinkerpop.utilities
plugin activated: tinkerpop.tinkergraph
gremlin> cluster = Cluster.open()
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
==>localhost/127.0.0.1:8182
gremlin>
```
I'll ask again that you test this more carefully and exhaustively. Our binary distributions can't have slf4j errors in it. Where are your tests of gremlin server, spark, giraph, and hadoop? do they log as needed?
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-200464637
I see, it's logging fine from the built artifacts but not for the assembly... need to fix `src/main/assembly/*.xml` as well
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-205250170
@ceefour it seems strange that you should have to touch the assembly files to get this to work properly. without putting much thought into it on my part, that doesn't seem like the right way to go. do you intend to return to this PR or should we just close this at this point?
Github user ceefour commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-205253990
@spmallette Sorry i forgot to actually make the change.
The assembly files were incorrect from the start since it only includes "compile" dependencies meaning "runtime" dependencies (which didn't exist before) were left out. I made the slf4j impl/log4j runtime or test dependencies since they're not required to compile (and users of the library can use any slf4j impl, not just log4j, i.e. me and Spring Boot )
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-215170072
It's been a few weeks, so I thought I would check-in - do you still plan to try to correct this PR?
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-221394103
I'm closing this for now as there hasn't been any movement on it in about a month. Please feel free to re-open when there is something that fully works.
GitHub user spmallette opened a pull request:
https://github.com/apache/tinkerpop/pull/373
TINKERPOP-1151 Made a number of changes to logging dependencies.
https://issues.apache.org/jira/browse/TINKERPOP-1151
Log4j is now generally a test dependency except for gremlin-server and gremlin-console where they need to be shipped as part of the binary distribution. In that case, they are optional scope for those who for some reason depend on those libs.
I tested this a bunch of different ways:
- `mvn clean install` shows the expected log messages
- `mvn clean install -DskipIntegrationTests` shows the expected log messages
- Gremlin Console displays log messages that aren't hidden by the default config and changes to that config allow logs to show in full
- Gremlin Server displays expected log messages
- Looked at the zip distributions and they obviously had the appropriate log4j jars
- Tested builds of both archetype outputs and logging is good within those
Anything else i missed where logging would be an issue? if not then VOTE +1 for me
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1151
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/373.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #373
commit 8bf021d28465b3cd88d66baaeacd380161f86086
Author: Stephen Mallette <spmva@genoprime.com>
Date: 2016-08-04T16:26:43Z
Made a number of changes to logging dependencies.
Log4j is now generally a test dependency except for gremlin-server and gremlin-console where they need to be shipped as part of the binary distribution. In that case, they are optional scope for those who for some reason depend on those libs.
Labelled this issue as "breaking" - not sure that anyone will notice a break but we do muck with dependencies a little bit so if someone was depending on TinkerPop to get their log4j dependency they'll have to make an adjustment to their pom. My PR mentions all this in the upgrade docs.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/373
Rebased and force pushed to resolve conflicts on master.
Github user twilmes commented on the issue:
https://github.com/apache/tinkerpop/pull/373
Looking good. VOTE: +1
Github user PommeVerte commented on the issue:
https://github.com/apache/tinkerpop/pull/373
Code looks straight forward. VOTE +1
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186237810
I guess we shouldn't have log4j be a standard dependency for `gremlin-core`, given use of slf4j, but I'm not sure making it a test dependency is right. we package log4j by default in the `gremlin-console` and `gremlin-server`. maybe those slf4j implementations should be added to those project directly and they are made a test dependency here?