Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1151

slf4j-log4j12 / log4j is only required for testing

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-incubating
    • Fix Version/s: 3.2.2
    • Component/s: build-release
    • Labels:

      Issue Links

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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"

        Show
        githubbot ASF GitHub Bot added a comment - 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"
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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`.

        Show
        githubbot ASF GitHub Bot added a comment - 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`.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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)


        Show
        githubbot ASF GitHub Bot added a comment - 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 )
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ceefour commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186455720

        Replaced by #233

        Show
        githubbot ASF GitHub Bot added a comment - Github user ceefour commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/229#issuecomment-186455720 Replaced by #233
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ceefour closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/229

        Show
        githubbot ASF GitHub Bot added a comment - Github user ceefour closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/229
        Hide
        githubbot ASF GitHub Bot added a comment -

        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

        Show
        githubbot ASF GitHub Bot added a comment - 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
        Hide
        githubbot ASF GitHub Bot added a comment -

        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....

        Show
        githubbot ASF GitHub Bot added a comment - 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....
        Hide
        githubbot ASF GitHub Bot added a comment -

        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

        Show
        githubbot ASF GitHub Bot added a comment - 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
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ceefour commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-196843308

        @spmallette Fixed

        Show
        githubbot ASF GitHub Bot added a comment - Github user ceefour commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/233#issuecomment-196843308 @spmallette Fixed
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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

        Show
        githubbot ASF GitHub Bot added a comment - 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
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        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 )

        Show
        githubbot ASF GitHub Bot added a comment - 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 )
        Hide
        githubbot ASF GitHub Bot added a comment -

        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?

        Show
        githubbot ASF GitHub Bot added a comment - 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?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/233

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/233
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.

        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        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.


        Show
        githubbot ASF GitHub Bot added a comment - 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.
        Hide
        spmallette stephen mallette added a comment -

        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.

        Show
        spmallette stephen mallette added a comment - 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.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the issue:

        https://github.com/apache/tinkerpop/pull/373

        Rebased and force pushed to resolve conflicts on master.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/373 Rebased and force pushed to resolve conflicts on master.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user twilmes commented on the issue:

        https://github.com/apache/tinkerpop/pull/373

        Looking good. VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the issue: https://github.com/apache/tinkerpop/pull/373 Looking good. VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user PommeVerte commented on the issue:

        https://github.com/apache/tinkerpop/pull/373

        Code looks straight forward. VOTE +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/373 Code looks straight forward. VOTE +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/tinkerpop/pull/373

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/373

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            ceefour Hendy Irawan
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development