Make pants a good example of Go contrib usage.

Review Request #3889 — Created May 14, 2016 and submitted

jsirois
pants
jsirois/issues/3424/buildgen-working
3424, 3443
3888
4bad0ce...
pants-reviews
benjyw, dturner-tw, kwlzn
This fixes the pants repo to be `buildgen` compatible and `buildgen`
tested. To do so, extraneous and `buildgen` illegal Go source roots used
for tests are eliminated and `buildgen` is added to CI to ensure
compliance with its strictures. Along the way a misconception about how
`go_remote_library` works is corrected in a test: in short, there is no
choice about where a `go_remote_library` BUILD file lives, it must live
at the remote repo "root".

 contrib/go/3rdparty/go/github.com/apache/thrift/lib/go/thrift/BUILD                   |  3 --
 contrib/go/examples/3rdparty/go/github.com/gorilla/context/BUILD                      |  4 ++-
 contrib/go/examples/3rdparty/go/github.com/gorilla/mux/BUILD                          |  4 ++-
 contrib/go/examples/3rdparty/go/gopkg.in/check.v1/BUILD                               |  7 +---
 contrib/go/examples/3rdparty/go/gopkg.in/fsnotify.v0/BUILD                            | 10 +-----
 contrib/go/testprojects/src/go/libUnstyle/BUILD                                       |  1 -
 contrib/go/testprojects/src/go/libUnstyle/unstyle.go                                  | 10 ------
 contrib/go/testprojects/src/go/libUnstyle/unstyle_test.go                             | 12 -------
 contrib/go/testprojects/src/go/usethrift/BUILD                                        |  5 ---
 contrib/go/testprojects/src/go/usethrift/example.go                                   |  8 -----
 contrib/go/testprojects/src/thrift/thrifttest/BUILD                                   |  4 ---
 contrib/go/testprojects/src/thrift/thrifttest/duck.thrift                             |  8 -----
 contrib/go/tests/python/pants_test/contrib/go/tasks/BUILD                             |  3 ++
 contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_test_integration.py       | 38 +++++++++++++++-----
 contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_thrift_gen_integration.py | 95 ++++++++++++++++++++++++++++++++++++++------------
 contrib/release_packages.sh                                                           |  2 +-
 pants.ini                                                                             | 14 ++++++--
 17 files changed, 126 insertions(+), 102 deletions(-)

Green locally:

./pants test contrib/go/tests/::
./pants test contrib/go/examples/::
./pants buildgen
./build-support/bin/release.sh -n

CI went green here:
http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3443/4/

JS
JS
BE
  1. 
      
  2. Out of curiosity, what was the necessity for replacing the testprojects/src/thrift files with this?

    1. I mean, as opposed to moving it to examples/.

    2. Consolidation on examples would have worked to be sure.  I am not a fan of that style of test as a test writer and reader though (facts are scattered about), and my guess is ~no users use them for examples unless maybe they have snippets linked in the docs.
    3. Makes sense.

  3. 
      
ST
  1. Enabling buildgen is a great step, thanks.

    Sidenote: I missed the boat on https://github.com/pantsbuild/pants/blob/master/contrib/go/README.md (it looks awesome!)... do you think it's ready to go on the main docsite with the JVM and Python sections?

    1. Looks like Benjy is hitting that here: https://rbcommons.com/s/twitter/r/3884/
    2. Correction: https://rbcommons.com/s/twitter/r/3891/
  2. contrib/release_packages.sh (Diff revision 1)
     
     

    Does running materialize in CI (ie, writing edits to disk) make sense? Or is that just the only way to get the failure for fail-floating?

    1. Since CI does do commits its harmless and at least exactly replicates what devs should be doing locally when modifying examples.
  3. pants.ini (Diff revision 1)
     
     
     
     

    Minor nit: these options are marked advanced, but they seem to be very relevant to manual, synchronous usage. Maybe remove them from advanced so they'll show in help by default?

    1. The idea behind advanced is they should be a choice for the repo and after that you should never be messing with them as a user on any given run.  Their default sense though is now debatable. The one known user - now 2 - both have to flip the flags this way.
  4. 
      
JS
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 6387414414ae171c038c5f59c2dbe2cb805b94c8
Author: John Sirois <john.sirois@gmail.com>
Date:   Sun May 15 10:24:10 2016 -0600

    Make pants a good example of Go contrib usage.
    
    This fixes the pants repo to be `buildgen` compatible and `buildgen`
    tested. To do so, extraneous and `buildgen` illegal Go source roots used
    for tests are eliminated and `buildgen` is added to CI to ensure
    compliance with its strictures. Along the way a misconception about how
    `go_remote_library` works is corrected in a test: in short, there is no
    choice about where a `go_remote_library` BUILD file lives, it must live
    at the remote repo "root".
    
    Testing Done:
    Green locally:
    ```
    ./pants test contrib/go/tests/::
    ./pants test contrib/go/examples/::
    ./pants buildgen
    ./build-support/bin/release.sh -n
    ```
    
    CI went green here:
      http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3443/4/
    
    Bugs closed: 3424, 3443
    
    Reviewed at https://rbcommons.com/s/twitter/r/3889/
Loading...