Fixing a few things before adding a feature to add a new item to MANIFEST.MF

Review Request #2005 — Created March 30, 2015 and submitted

zundel
pants
zundel/normalize-some-build-files
1343
f8ccd63...
pants-reviews
jsirois, patricklaw
- Updated BUILD files for java/distribution and java/jar to more closely follow conventions
- Adds a unit test for the Manifest class.

Added a unit test, ran through CI (see Bugs link). +.01% coverage!

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
ZU
ZU
  1. 
      
  2. Minor thing, I could have written this as

    src/python/pants/java/distribution
    

    but we rarely do it that way unless we mean a target that is referring to a bunch of things.

    If you look at the directory for src/python/pants/java/distribution This is an unusual case in a couple of ways. This is really just a single module that we put into a directory because it has a sidecar java class that lives with it. Also, the source file is 'distribution.py' an its in a directory called 'distribution' so referring to the target is the same as referring to the default target. I thought about renaming either the directory or the source module it's a bit awkward no matter how we do it.

  3. 
      
NH
  1. Thanks for making things more consistent. I've got a couple nitpicks.

  2. If you are going to use distribution:distribution, could you change this reference here to make it consistent?

  3. src/python/pants/java/BUILD (Diff revision 2)
     
     

    Also here.

    1. Minor difference here: this one is intended to reference all targets in the directory, so I'm going to leave it as-is. (Although admittedly there is actually only one target!)

    2. ok.

    3. In retrospect, I shouldn't have added this new target at all, so I removed it. This is something we do in the unit tests, but only so we can call all the unit tests and skip over the integration tests. We don't want to replicate this pattern everywhere.

  4. 
      
ZU
NH
  1. Ship It!
  2. 
      
PA
  1. 
      
  2. Internally our style (and buildgen's output) always chooses the minimal representation. I'm somewhat indifferent, though if asked I would lean toward the minimal spec.

    1. My thought about this was that if we ever add more targets to src/python/pants/java/distribution/BUILD then this dependency may pull in more than intended at this point. I'm thinking you guys probably follow the 1:1:1 rule so this might not be an issue for you internally.

    2. I'm talking specifically about the trailing :distribution. I'm not aware of any convention saying that eliding the trailing distribution means anything other than "this is shorthand for X". And if we ever add more targets here, the meaning of the spec doesn't change.

      Like I said, I'm pretty indifferent. But I don't think on relying on the shorthand (but canonical) spec to be meaningful is valuable, and might be actively harmful if it's only half-heartedly enforced.

    3. In the Pants test infrastructure, the convention is that the default target is a list of alll targets defined in the file to run the unit tests. Granted, this is in the source of pants itself, not the tests...

  3. 
      
JS
  1. Not a blocker - I just don't understand the target(...)s added here.
  2. src/python/pants/java/BUILD (Diff revision 3)
     
     
    I am not a fan of this target or src/python/pants/java/jar.  It seems to me our production source tree should be moving towards 1:1:1 in which case the target() -> python_library for both of these targets using a globs('*.py') or else both of these targets should go, no-one should use the shortcut / fat target and instead they should point at the thin/exact targets.
    1. I think I was extending the convention we have in the tests/ directory for unit tests into the source directory which in retrospect is a mistake. I'll get rid of it and the one that Patrick pointed out.

      I don't understand the motivation for the 1:1:1 model fully and how we have been working toward it up to this point. My understanding is that is one target per BUILD file and source files only in leaf directories. We would have to create hundreds of directories to support it? Or collapse our fine grained targets into courser grained targets? That seems to be contrary to your last sentence above.

      I am also fine with moving 'manifest.py' down into the java directory and getting rid of the jar directory if you want to get rid of some complexity that way.

    2. My comment was meant to not favor 1:1:1 or thin, but to suggest commitment to one or the other.  I'm happy enough with you getting rid of the tests/ pattern mimick you suggest.
      
      I will say 1:1:1 is generally easy to reason about, and tends to mirror _jvm_ units of encapsulation - python not so much.  The extreme fine-grained targets we have now were the result of 1 big change by Wickman before your time [1] when he got frustrated with the circular deps hidden by what was at the time twitter/commons/pants coarser and fewer targets.  We may be able to pull back from this extreme style of 1-file targets now that cycles are gone.
      
      [1] https://rbcommons.com/s/twitter/r/32/
  3. 
      
ZU
JS
  1. Ship It!
  2. 
      
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews Nick, Patrick and John. Commit ce9f91e

Loading...