Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-861

Fix or document issue where first time user cannot use mvn compile

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: next
    • Component/s: None
    • Labels:
      None

      Description

      Having come up from last vote:

      While I managed to get it to compile, note that a “mvn compile” will fail with:
      Could not find artifact org.apache.calcite:calcite-avatica:jar:tests:1.4.0-incubating in central

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          This is already documented. README refers to doc/howto.md, whose first section is "Building from a source distribution".

          Show
          julianhyde Julian Hyde added a comment - This is already documented. README refers to doc/howto.md, whose first section is "Building from a source distribution".
          Hide
          elserj Josh Elser added a comment -

          From the mailing list, I'll try to take a look at this tonight to see if we can remove the need to special usage.

          Show
          elserj Josh Elser added a comment - From the mailing list, I'll try to take a look at this tonight to see if we can remove the need to special usage.
          Hide
          elserj Josh Elser added a comment -

          I think the general problem is that the test-jar isn't available to the reactor (thus other modules only have access to it if it's been installed). I believe the recommended approach would be to put these into src/main. To prevent these classes from "leaking" out to users, we can just make a new module which the other modules only depend on at the test scope.

          Show
          elserj Josh Elser added a comment - I think the general problem is that the test-jar isn't available to the reactor (thus other modules only have access to it if it's been installed). I believe the recommended approach would be to put these into src/main. To prevent these classes from "leaking" out to users, we can just make a new module which the other modules only depend on at the test scope.
          Hide
          elserj Josh Elser added a comment -

          Sooo, this took a lot longer than I originally expected.

          I at least has the tree building again, but need to put in some more time tmrw to make sure it's actually a positive step in the right direction. I plan to have a PR open tmrw.

          Show
          elserj Josh Elser added a comment - Sooo, this took a lot longer than I originally expected. I at least has the tree building again, but need to put in some more time tmrw to make sure it's actually a positive step in the right direction. I plan to have a PR open tmrw.
          Hide
          julianhyde Julian Hyde added a comment -

          What you are recommending is consistent with what I found: http://stackoverflow.com/questions/1725476/maven-test-dependency-in-multi-module-project

          Still can't quite believe that the reactor can't handle test artifacts. But anyway.

          When moving "common test infrastructure" to a new module, is there clearly a right place to cut? If cti depends on core and core-test depends on cti, isn't that a cyclic dependency? Is that a problem for maven?

          Show
          julianhyde Julian Hyde added a comment - What you are recommending is consistent with what I found: http://stackoverflow.com/questions/1725476/maven-test-dependency-in-multi-module-project Still can't quite believe that the reactor can't handle test artifacts. But anyway. When moving "common test infrastructure" to a new module, is there clearly a right place to cut? If cti depends on core and core-test depends on cti, isn't that a cyclic dependency? Is that a problem for maven?
          Hide
          elserj Josh Elser added a comment -

          Still can't quite believe that the reactor can't handle test artifacts

          Yeah, I'm not sure why this is. I would think that as long as you invoke something that hits the test maven lifecycle phase, you'd have access (which means compile would still not work), but that doesn't seem to be the case either. :shrug:

          When moving "common test infrastructure" to a new module, is there clearly a right place to cut? If cti depends on core and core-test depends on cti, isn't that a cyclic dependency?

          Yes, this is the pain in doing it. The easiest thing I found was to just move the tests from calcite-core wholesale to a calcite-test module. I had started trying to just pick the ones that were absolutely necessary to move (because other downstream modules had dependencies on them), but that was a futile effort.

          I imagine it would be easy to move the limited tests back to calcite-core after I get all of the tests running (a few are throwing exceptions).

          Another option would be to put the test classes into calcite-core/src/main/java and configure surefire to look in src/main/java instead. This would have the negative impact of bloating the size of the jar though.

          Show
          elserj Josh Elser added a comment - Still can't quite believe that the reactor can't handle test artifacts Yeah, I'm not sure why this is. I would think that as long as you invoke something that hits the test maven lifecycle phase, you'd have access (which means compile would still not work), but that doesn't seem to be the case either. :shrug: When moving "common test infrastructure" to a new module, is there clearly a right place to cut? If cti depends on core and core-test depends on cti, isn't that a cyclic dependency? Yes, this is the pain in doing it. The easiest thing I found was to just move the tests from calcite-core wholesale to a calcite-test module. I had started trying to just pick the ones that were absolutely necessary to move (because other downstream modules had dependencies on them), but that was a futile effort. I imagine it would be easy to move the limited tests back to calcite-core after I get all of the tests running (a few are throwing exceptions). Another option would be to put the test classes into calcite-core/src/main/java and configure surefire to look in src/main/java instead. This would have the negative impact of bloating the size of the jar though.
          Hide
          julianhyde Julian Hyde added a comment -

          Now we know the reason for this, could we not simply document the limitation: it is not sufficient to type mvn install, you have to type mvn -DskipTests install && mvn test. Or mvn clean && mvn -DskipTests install && mvn test if you want to clean first.

          It's not too difficult a workaround.

          Show
          julianhyde Julian Hyde added a comment - Now we know the reason for this, could we not simply document the limitation: it is not sufficient to type mvn install , you have to type mvn -DskipTests install && mvn test . Or mvn clean && mvn -DskipTests install && mvn test if you want to clean first. It's not too difficult a workaround.
          Hide
          elserj Josh Elser added a comment - - edited

          could we not simply document the limitation

          Yes, we could.

          I just opened https://github.com/apache/calcite/pull/151 which works around the fundamental problem. IMO, relying on test-jars to share classes is hacky, as is shown by having to use special Maven incantations. Long term: I think reorganizing the code is the correct solution, whether or not this happens for 1.5.0 is another question.

          Show
          elserj Josh Elser added a comment - - edited could we not simply document the limitation Yes, we could. I just opened https://github.com/apache/calcite/pull/151 which works around the fundamental problem. IMO, relying on test-jars to share classes is hacky, as is shown by having to use special Maven incantations. Long term: I think reorganizing the code is the correct solution, whether or not this happens for 1.5.0 is another question.
          Hide
          julianhyde Julian Hyde added a comment -

          The way I see it, Maven artifacts work fine (you can register main, test, javadoc, test-javadoc etc. artifacts in a repository, and they can depend on each other) but it's just the reactor that is broken. If the reactor worked OK we wouldn't even be having this conversation about whether one test artifact can depend on another. Obviously it should be OK.

          Now imagine someone who has just downloaded Calcite who can't find the tests because they are in another module. I can imagine an IDEs that can efficiently rebuild and run tests within the same module but require a longer build process to rebuild a different module than is being tested.

          People will be tempted to start introducing dependencies on test libraries (jmockit, junit) into core, which would be bad.

          It's not unreasonable to expect people to look in the doc to find out that the build command is mvn clean && mvn -DskipTests install && mvn test. I'd rather that than 150 files moved to an unintuitive location.

          Show
          julianhyde Julian Hyde added a comment - The way I see it, Maven artifacts work fine (you can register main, test, javadoc, test-javadoc etc. artifacts in a repository, and they can depend on each other) but it's just the reactor that is broken. If the reactor worked OK we wouldn't even be having this conversation about whether one test artifact can depend on another. Obviously it should be OK. Now imagine someone who has just downloaded Calcite who can't find the tests because they are in another module. I can imagine an IDEs that can efficiently rebuild and run tests within the same module but require a longer build process to rebuild a different module than is being tested. People will be tempted to start introducing dependencies on test libraries (jmockit, junit) into core, which would be bad. It's not unreasonable to expect people to look in the doc to find out that the build command is mvn clean && mvn -DskipTests install && mvn test . I'd rather that than 150 files moved to an unintuitive location.
          Hide
          elserj Josh Elser added a comment -

          If the reactor worked OK we wouldn't even be having this conversation about whether one test artifact can depend on another. Obviously it should be OK.

          I'd love to find an answer about this. Perhaps I need to shoot a note over to their user's list.

          can't find the tests because they are in another module

          IMO, this isn't that confusing given my breadth of projects. It's pretty common to see lots of tests all over the place with varying degree. While wholesale moving all of the tests from core/ elsewhere is a big impact, there are likely some restricted-in-scope tests which still should live in core/. I just didn't have the patience to try to move them back yet.

          Personally, I find test classes depending on other test classes from another module to be confusing, but perhaps that's because I know it's a smell with Maven.

          doc to find out that the build command is

          The docs already state the following to build the source distribution. Given your obvious desire to not make these changes, I don't think there's anything that needs to happen here.

          Show
          elserj Josh Elser added a comment - If the reactor worked OK we wouldn't even be having this conversation about whether one test artifact can depend on another. Obviously it should be OK. I'd love to find an answer about this. Perhaps I need to shoot a note over to their user's list. can't find the tests because they are in another module IMO, this isn't that confusing given my breadth of projects. It's pretty common to see lots of tests all over the place with varying degree. While wholesale moving all of the tests from core/ elsewhere is a big impact, there are likely some restricted-in-scope tests which still should live in core/. I just didn't have the patience to try to move them back yet. Personally, I find test classes depending on other test classes from another module to be confusing, but perhaps that's because I know it's a smell with Maven. doc to find out that the build command is The docs already state the following to build the source distribution. Given your obvious desire to not make these changes, I don't think there's anything that needs to happen here.
          Hide
          elserj Josh Elser added a comment - - edited

          Actually, the section covering running tests does miss actually invoking `mvn test`

          https://github.com/apache/calcite/pull/153 is a trivial change to be explicit in this.

          Show
          elserj Josh Elser added a comment - - edited Actually, the section covering running tests does miss actually invoking `mvn test` https://github.com/apache/calcite/pull/153 is a trivial change to be explicit in this.
          Hide
          elserj Josh Elser added a comment -

          https://maven.apache.org/plugins/maven-jar-plugin/examples/create-test-jar.html#The_preferred_way outlines how Maven recommends shared test classes are handled.

          tl;dr Create a module for testing (to manage test-dependencies in one place), move test resources from src/test/java to src/main/java, include that module in the test scope.

          This is very close to what I did initially, fwiw.

          Show
          elserj Josh Elser added a comment - https://maven.apache.org/plugins/maven-jar-plugin/examples/create-test-jar.html#The_preferred_way outlines how Maven recommends shared test classes are handled. tl;dr Create a module for testing (to manage test-dependencies in one place), move test resources from src/test/java to src/main/java, include that module in the test scope. This is very close to what I did initially, fwiw.
          Hide
          julianhyde Julian Hyde added a comment -

          I have merged Josh Elser's doc change in http://git-wip-us.apache.org/repos/asf/calcite/commit/b94a00e3. That does not fix this issue, however.

          Show
          julianhyde Julian Hyde added a comment - I have merged Josh Elser 's doc change in http://git-wip-us.apache.org/repos/asf/calcite/commit/b94a00e3 . That does not fix this issue, however.

            People

            • Assignee:
              elserj Josh Elser
              Reporter:
              jnadeau Jacques Nadeau
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development