Separate compile step for C++ to just compile objects

Review Request #1855 — Created March 3, 2015 and submitted

dma
pants
53e6618...
pants-reviews
dturner-tw, jsirois

The 'compile' goal applied to a cpp_library or cpp_binary target will just compile the source files into object files. The products of this are then used to link libraries and binaries.

PR: https://github.com/pantsbuild/pants/pull/1195

ran the tests

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
DT
  1. 
      
  2. Why is this a function?

    1. hangover from the old version plus some thinking about separate platforms having different naming and the possibility of shared libraries in the future.

      but i should keep this simple for now :)

  3. Maybe this should just raise an unimplemented exception? That is, maybe this should be an abstract class?

    1. My bad, I was misreading the diff.

    2. added CppTask.execute that raises NotImplementedError

  4. 
      
DM
DM
ST
  1. 
      
  2. Should this be in a deeper namespace to avoid collision with non-CC objects?

    Or is this intended to be a new "top-level" pants product that multiple languages might participate in? (I recognize that this is more of a question for cc jsirois)

    1. In the new world product types with be types (they could be today! you could do this now) and have the namespace we get for free with a packaged class so I'm not too concerned about the handful - O(50) - string keys that exist.
  3. 
      
JS
  1. I've marked the 2 bits that really must be fixed as issues, other bits could be fixed in follow on RBs.
  2. s/objects = /objects.extend(...)/ - you throw away all but the last set of items in the loop right now.

  3. Absolutely; dep.workdir should die. My bad for flagging this in the prior RB and then failing to stick to it in review.

    1. i tried this but couldn't get it to work. i'll leave it as a TODO for now and work on it next.

  4. This is a gratuitous lie for laughs - you compile just what needs compilation.  Consider a more appropriate comment.
  5. Its a confusing split today, but require is for products.get and require_data for products.get_data. It works because products use a global key namespace whether a ProductMapping or data, but this should use require to keep things clear for the refactor that merges the 2 product systems into one.

  6. It would be nice to have the integration tests in files parallel to the tasks they test.  Benefits are better shardability and a more standard path to test discovery for humans.  This is probably fine for now, but if you're not averse a TODO and/or issue would be good.
  7. 
      
DM
JS
  1. 
      
  2. A minor thing for future - you can omit the bracketing [] around the generator and thus skip building a list, extend can handle the raw generator since it only cares that it gets an iterable thing.
  3. 
      
JS
  1. In master @ https://github.com/pantsbuild/pants/commit/ac23fb8185f77084c9dfe2794d233ad9db19714f
    Please mark this review as submitted.
  2. 
      
DM
Review request changed

Status: Closed (submitted)

Loading...