C++ support in contrib

Review Request #1818 — Created Feb. 23, 2015 and submitted

benjyw, dturner-tw, jsirois

This patch allows for C++ projects to use pants to build statically linked libraries and binaries using the 'binary' goal. It also adds to the 'run' goal such that binaries can be run from within pants.

integration tests run and manual testing of 'binary' and 'run' goals on cpp_library and cpp_binary targets.

  • 0
  • 0
  • 6
  • 2
  • 8
Description From Last Updated
  1. Thanks again for putting this all together and then moving things around.
    The biggest missing bit here is having the library task and the binary task each store their outputs in the Context Products.  Once you do that, binary can become decoupled from the details of library and run from binary - they can just require and pluck upstream products from the context and do their work against those.
  2. contrib/BUILD (Diff revision 1)
    If its the case that aligning directory structure with cpp namespace structure is typical, then it would be good to see a src/cpp source_root here and good to see namespace alignment with dir structure in the examples.  I'm thinking of a task that bundles up headers for creation of a "dev" package.
    1. i don't think i understand. do you mean add a 'cpp/examples/src/cpp' source root and then C++ namespace { matching the directory structure under example?

    2. Yes - that's what I mean.  Java by strong convention and python by design have this alignment.  Scala allows an arbitrary mapping between fs nesting and namespaces as does cpp, what I'm unclear on is if the c++ universe also has a strong convention like java to align and not go all arbitrary.  If their is a strong convention then I was suggesting following it in the example, if not - I withdraw the 'namespace alignment' suggestion.  The source_root suggestion stands though.
    3. there's not a strong convention but it's a general good practice. i can absolutely add it to the c++ example.

      for the source_root, that changes where the include paths end up being set from target_base. this is unexpected as the root of the include path should be where the build file is.

  3. We tend to leave off the target name in the address when it's the default name (s/:hello//).
  4. You use python kwarg style in BUILD files above - which I like - but here and in some BUILDs below you pad the kwarg = with space.  Pick one style.
  5. Note to self, I need to expose an object in the context that allows all this to be replaced with:
      description='C++ pants plugin.',
  6. I would imagine this being installed in 'compile', was this a typo or intentional?
    1. this is intentional. a library is a 'compile' and 'link' so i put it under binary. that way, a C++ developer doesn't need to know if they're working with libraries or binaries, they just 'pants binary ...'

  7. No labels needed on new targets, we've got it on the docket in the last few summits to kill these globally.
  8. 2 space indents - this file is using 4.
    Also - this is minor - but there is a tension between Target (noun) and Task (verb).  In the general case many verbs can act on a given noun.  For example: compile, doc & checkstyle can all be applied to a java library and they all produce different products.  As such its generally more appropriate to document what a Target represents and not what might be created from it.
  9. Since you're using the standard sources payload, your targets are source_root sensitive.  It would probably be good to model the contrib examples tree with a source_root and ... I'm less confident here ... namespaces that mirror directory structures under the source root.
    1. ah so this is refering to your point above. i still don't fully understand what you mean by namespaces and mirroring directory structure.

    2. Hopefully I explained myself above more clearly.
  10. Please kill t.c.log - I removed this globally here: https://rbcommons.com/s/twitter/r/1815/
    Instead just use the self.context.log since you're a Task.
  11. You don't actually populate products in the context so this is not needed.  You may intend to do this though, in which case a TODO / issue is probably fine.
    1. this is necessary for cpp_run to make sure the binary is built before trying to run it.

  12. No need to ever do a pure pass-thru like this - kill.
  13. Please don't mutate targets - we'd like to keep the graph frozen after it's read from disk early on.  We're working on making the immutability more manifest, but for now its just the spirit of things.  The concept to use here is self.context.products.  The CppLibraryCreate Task below should be populating a 'lib' product with a data structure that allows downstream tasks to find and use the libs it created.
    1. do you have a good example of this that i can learn from?

    2. Sorry for not providing links or guidance - that was a big miss.

      Here are the 2 relevant high-level doc sections:
      + http://pantsbuild.github.io/dev_tasks.html#product_types-and-require_data-why-test-comes-after-compile
      + http://pantsbuild.github.io/dev_tasks.html#products-map-how-one-task-uses-products-of-another

      And here is an example of JarCreate's 'jars' products being consumed by XXX without regard to both who produces the jars or where they live:

      + Advertise 'jars' product: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jar_create.py#L57
      + Populate 'jars' product mappings: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jar_create.py#L75

      + Require 'jars' product: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jar_publish.py#L408
      + Consume 'jars' product:

  14. The workunits allow you to request named streams and pass these into subprocesses in order to capture their output.  You can see the upshot by running the pants server: `./pants server` - and then a pants command that shells out like you do here: `./pants clean-all compile examples/src/java::`.  You can navigate the tree for the latest run in the web console to items with a gear icon and see the full command line, stdout and stderr.  An example of setting this all up is here in PythonEval: https://github.com/jsirois/pants/blob/master/src/python/pants/backend/python/tasks/python_eval.py#L160-160
    You might play with it's use of of an 'MULTITOOL' workunit enclosing the loop over the N compiles.  This has an effect on both the console output and the server console output that might be desirable.
    1. i'm going to punt on this for now.

    2. Please add a TODO then for at least the stdout/stderr plumbing.  Any stdout/stderr "leaks" in the console output are noticed and corrected eventually.
    3. done

  15. See PythonRun for passthrough arg handling (pants allows for ./pants run contrib/cpp/examples/src/cpp/example:hello_pants -- my pass through args

  16. Consider 'extensions' and maybe clarify - without the . - for a more common terminology.  At least I think suffix will be less surprising to most readers.
  17. kill - chaining is automatic through supers if omitted.
  18. A common-enough idiom is _ for a "don't care" variable name, so _, ext = ... would be fine here and you have another - opposite sense - instance below in the _compile method.

  19. Slightly odd to have ALL_CAPS for args here since its not a ~constant.
    1. it's constant as in not written to...

      i could just kill it though. it doesn't add that much.

    2. Sure, you'll just not find any ALL_CAPS local variables in pants python (or ~any?) python code.  It just stands out as odd.
    3. done

  20. isinstance instead please - kill label usage.
  21. Tasks should raise TaskErrors [1].  You can subclass to add extra info (possibly for tests).
    [1] https://github.com/jsirois/pants/blob/master/src/python/pants/base/exceptions.py#L9-9
  22. We're happy with contracting to `_compiler = compiler or os.environ.get('CXX')` for these cases.
  23. I don't think you can hit this condition given register_tool internals above,
  24. s/python_test_suite/target/ - I've added myself a TODO to rid the bad python_test_suite examples globally: https://github.com/pantsbuild/pants/issues/1161
  25. These are TODO-before-submitting and need to be addressed... Well, not really since pants depends on python libs like psutil with C extensions that need to be compiled already, but...
  26. Just `import unittest` please - killed all examples like you have here a while back since pants restricted its runtime to python2.7 a ways back.
  27. Please don't modify the environ globally in this test or assume even 'ls' is on the PATH. Instead you can use the environment_as contextmanager to setup scoped environ mutations: https://github.com/jsirois/pants/blob/master/src/python/pants/util/contextutil.py#L23

    You could also use the temporary_dir [1] contextmanager, touch [2] and chmod_plus_x [3] to setup known good tools.

    [1] https://github.com/jsirois/pants/blob/master/src/python/pants/util/contextutil.py#L51
    [2] https://github.com/jsirois/pants/blob/master/src/python/pants/util/dirutil.py#L161-161
    [3] https://github.com/jsirois/pants/blob/master/src/python/pants/util/dirutil.py#L128-128

  1. I think it would be good to add (back) more reviewers.  I'm not sure if you intend to do that after straightening most things out with me or not, but suggestions are Benjy Weinberger (ex goog c++ guy, so lots of relevant experience), David Turner (Twitter c guy).
  2. This can go - its already defined in CppTask below.
  3. This is a scope error waiting to raise - either self.is_library or CppBinaryCreate.is_library - I prefer self.
  4. No \ needed here and we globally try to format code to avoid them when otherwise needed - generally by introducing parens to enclose the continuation as a last resort.
  5. Another NameError here, self.is_lbrary(dep).

  1. This all LGTM but I'd like one more sign-off before patching in.  Please add back at least some of the reviewers from https://rbcommons.com/s/twitter/r/1581/
    1. Oops - you already did add more reviewers - ty.
  2. Note that https://github.com/pantsbuild/pants/commit/7137ddf564686edbdb5e6c81dcf5c16f3e0e522f has now landed on master and you can replace this with the following if you rebase:

      description='C++ pants plugin.',
    1. done. thank you!

  1. In case its too buried in RB comments, I asked Dominic to add you Benjy and David for extra eyes on this 1st real contrib.  If either of you doesn't have bandwidth, please suggest a replacement reviewer.
  2. I think libraries needs to be part of the payload for correct fingerprint generation/invalidation.

  3. I actually think that there should be two separate tasks: compile and link. That way, if I decide to change what I link against, I don't have to rebuild all my objects. But again, this is an optimization.

    1. I have a TODO for that in terms of adding a 'compile' task to the 'compile' goal and then having the 'binary' tasks (library and binary) depend on the object products from that.

  4. Maybe I'm reading this wrong, but I think this potentially does lots of extra compilation.

    Imagine that you have a cpp_binary() with sources main.cc and aux.cc. aux.cc changes. main.o is still fine, right? But here, it would have to be recompiled. The traditional strategy in e.g. make(1) is to only regenerate .o files for .cpp files which have changed (or whose headers have changed). Gcc, at least, can generate this dependency information for you (with -M).

    Pants's java compilation uses jmake's analysis to do this minimal recompilation.

    I guess you could track this information yourself in your BUILD files, which would mostly work. But that seems extremely error-prone.

    I'm opening an issue here, but I won't block a Ship It on this, because you presumably decided it was better to have some c++ support than none (and I probably agree). But please at least add a TODO.

    1. I agree. There's no firm strategy for dealing with this well. gcc, as you say, can generate the dependency information but parsing it is left up to the build tool. Adding that support as a second pass seems like a good immediate step to take. TODO added.

  2. I think it's not a SourcesField -- that would mean that e.g. there is a source file named 'm' or 'rt'. I think you want a ConfigurationsField?

    Also, if your tests didn't crash here, then you need more tests.

    1. added a new test (which crashed in two places. thank you) and used PrimitiveField which seems more correct.

  1. Ship It!
  1. I'm happy to have had another set of eyes - I'm going to patch this in since there will be iteration and more change for folks to chime in then.
    1. In master @ https://github.com/pantsbuild/pants/commit/c36c93895085cc6553acba725527bab165a07d35
      Please mark this RB submitted.
      One note post-facto - the Summary and Description form the commit message when doing `rbt patch -c [RB_ID]`.  Going forward its best to have your description be meaningful as a commit message.  For this Rb I just deleted the description.
    2. I actually had to revert @ https://github.com/pantsbuild/pants/commit/532a83712c9d2162ecc2aeeb08447c4035abdbf3
      This RB is broken in CI - see here: https://travis-ci.org/pantsbuild/pants/builds/52493151
      There are both isort issues and a problem with target addresses in your contrib tests.
    3. i figured out that i can run the tests locally. i'm not sure why the ci is different to PANTS_DEV=1 ./pants test ..../cpp:integration but at least i can repro.

      i'll have a new patch soon.

    4. https://travis-ci.org/pantsbuild/pants/builds/52503719

      green build :)

  1. In master @ https://github.com/pantsbuild/pants/commit/5d918a9bf35fd22cbd3c422b7fb65f3aca48249d
    Please mark this review as submitted.
Review request changed

Status: Closed (submitted)