Remove defaults for custom scala tools.

Review Request #3609 — Created March 25, 2016 and submitted

2961, 3091
benjyw, gmalmquist, jsirois, patricklaw, stuhood, wisechengyi

Remove defaults for custom scala tools.

Previously there were defaults for 'custom' jvm tools that caused problems if the user specified custom
but did not create BUILD files for each needed target. Troubleshooting this problem was difficult
because pants did not fail.

  • Add integ tests to test scala-platform code.

CI green:

Added integ tests to verify that custom targets fail when underspecified and work correctly when specified.
Local testing on testprojects target.

  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
  2. src/python/pants/backend/jvm/subsystems/ (Diff revision 1)

    This is the actual fix, right? By using None as default, exception will be thrown down the line if they are not set?

  3. It might be better to name them for being consistent with and easy to understand.

    1. using would cause it to be interpreted by pants as a build file in that target... we don't want this.

  4. integ -> integration in description and files.

  2. Does the failure give a readable error message? The change I saw above didn't seem to add a specific handler to catch when the found spec was None.

    1. for scalac its better than it was previously in that you get an exception. I'll add a specific useful exception in the bootstrap code in the case that it tries to bootstrap None.

      09:45:11 00:02 [bootstrap-scalac]
      ==== stderr ====
      Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=256m; support was removed in 8.0

                         ==== stdout ====
                         :: loading settings :: file = /Users/molsen/workspace/pants/build-support/ivy/ivysettings.xml
                         :: resolving dependencies :: internal#f0810a1855271ee7f7411ce22aac46aaa0c0dd3c-IvyResolveFingerprintStrategy_53c4bb9c6553-resolve;working@tw-mbp-molsen
                          confs: [default]
                         :: resolution report :: resolve 1076ms :: artifacts dl 0ms
                          |                  |            modules            ||   artifacts   |
                          |       conf       | number| search|dwnlded|evicted|| number|dwnlded|
                          |      default     |   1   |   0   |   0   |   0   ||   0   |   0   |
                         :: problems summary ::
                         :::: WARNINGS
                              module not found: org.scala-lang#scala-compiler;None
                          ==== maven-central: tried
                          ==== tried
                            -- artifact org.scala-lang#scala-compiler;None!scala-compiler.jar:
                          ==== tried
                            -- artifact org.scala-lang#scala-compiler;None!scala-compiler.jar:
                          ==== local.m2: tried
                            -- artifact org.scala-lang#scala-compiler;None!scala-compiler.jar:
                              ::          UNRESOLVED DEPENDENCIES         ::
                              :: org.scala-lang#scala-compiler;None: not found

    2. Because they are handled slightly differently you get two slightly different messages. For the bootstrapped tools that ScalaPlatform fetches a classpath you would see something like:

                     compile(testprojects/src/scala/org/pantsbuild/testproject/scalac/plugin:plugin) failed:
                             Unable to bootstrap tool: 'scalac' because no rev was specified.  This usually
                             means that the tool was not defined properly in your build files and no
                             default option was provided to use for bootstrap.

      For tools that return the spec to be inserted into the buildgraph you would see something like:

      Exception message: scala-library was not found in BUILD files from /Users/molsen/workspace/pants/. Perhaps you meant one of:
      :readme (from BUILD)
      :scala-js-cli (from
      :scala-js-compiler (from
      :scala-js-library (from
      :scrooge-gen (from
      :scrooge-linter (from
      when translating spec //:scala-library
      referenced from testprojects/src/scala/org/pantsbuild/testproject/scalac/plugin:plugin

  2. Can you elaborate on what this is checking? When is this scope used, and what does it imply?

    1. This is to make sure that this logic doesn't impact any tools other than ScalaPlatform tools.

      Scope comes from the "register" object from the scope of where register_jvm_tool is called. For Scala Tools this will be scala-platform.

  3. This is a terrifying type check. Why not just ask if t isinstance JarLibrary? Even then, why is the check needed?

    Also, while I'm usually a fan of generator expressions, here I'd prefer to see list comprehensions. If later on someone adds another check that references these variables, they will first exhaust the generator and then get confusing results on the second use.

    1. Regarding the type_alias check I need to know if this target was constructed in memory or not. Happy to change the generator expresion to a list comprehension.

      From the type_alias docstring:

      """Returns the type alias this target was constructed via.
      For a target read from a BUILD file, this will be target alias, like 'java_library'.
      For a target constructed in memory, this will be the simple class name, like 'JavaLibrary'.
    2. I'm still uncomfortable with this check. This is just to support testing?

    3. This is just to allow the user to get a nicer error message if they don't specify a custom target when needed. Otherwise they get an error message about not being able to do an ivy resolve for a None package. The full error is in one of the previous comments. With or without this code the user still gets an error... one is just harder to decipher.

    4. Sorry, I get the empty revs check and how this is generated a nicer error, but I'm still not clear on the purpose of the alias/synthetic check--which seems very fragile to me.

    5. Depending on whether or not the target has been created as part of the jvm tool registration or not you will get different type alias's. If we have reached this point in the code and rev is None then we are trying to bootstrap a custom scala tool target. If this is an on disk build file target then everything is fine and we move on... if its not then the user didn't specify a build target and we need to provide a useful message.

      As far as the synthetic target, this is just to make writing tests that insert synthetic targets reasonable.

  4. Can we use a typed exception? And add a test to exercise the specific exception, ideally.

    1. Will do, I already have some tests that make sure an exception is raised, I'll just check for a more explicit exception.

  2. src/python/pants/backend/jvm/tasks/ (Diff revision 5)

    Does this need a dedent?

    1. added, looks better with the dedent.

  2. Is this check necessary? It seems plausible that other platforms could also have this problem, and it's also a code smell for generic jvm tool bootstrapping to have an awareness of scala platform specific logic.

  2. It's good to avoid unnecessary diffs. Also, the indentation of the trailing ) is off. Personally, I strongly prefer dedenting all the way back rather than the dangling indent.

  3. Nit: "it's"

Review request changed

Status: Closed (submitted)

Change Summary:

commit 9212fe0a84769012745ebc91db7bf138aff32e93