Document some of sh_binary (including my confusion about the implementation)

Summary:
This thing is crazy. The implementation makes no sense. It's doing
some things that are just really strange and then it's doing some similar
things in two completely different ways (one of which is really complex).

I've probably spent hours now trying to figure out wtf this does. I'm adding
some documentation and commentary to help me or the next person waste less
time.

Reviewed By: ndmitchell

fbshipit-source-id: e967ab5de27f9f9f9f37fc7e4a01cfc5b22e0142

Author: cjhopman

src/com/facebook/buck/shell/ShBinary.java

@@ -67,6 +67,12 @@
 import java.util.logging.Level;
 import java.util.stream.Stream;
 
+// TODO(cjhopman): Don't allow people to just write complex, hard to follow behavior without
+// documenting it.
+// TODO(cjhopman): Figure out wtf this code does. It's creating symlinks all over the place, some of
+// them within this things own getBuildSteps, others by passing information into the generated
+// template and then creating them when invoked. It looks like it's creating multiple symlinks for
+// everything that appears in resources. Why does it do that? And where are they going?
 public class ShBinary extends AbstractBuildRuleWithDeclaredAndExtraDeps
     implements BinaryBuildRule, HasRuntimeDeps {
 
@@ -185,6 +191,8 @@ public ImmutableList<Step> getBuildSteps(
     valuesBuilder.put("cell_names", cellsNamesBuilder.build());
     valuesBuilder.put("cell_paths", cellsPathsStringsBuilder.build());
 
+    // TODO(cjhopman): Why is this so similar to the resource path construction above? What's the
+    // difference and why?
     Path defaultRuntimeResourcesPath =
         runtimeResourcesDir
             .resolve(ROOT_CELL_LINK_NAME)

src/com/facebook/buck/shell/sh_binary_template

@@ -1,3 +1,17 @@
+<!
+This template creates a shell script that invokes a user shell script in a simple sandbox.
+
+Expects the following template arguments:
+
+path_to_project_root_file - path relative to this script's directory to the "project_root" file
+    in buck-out. That file is expected to contain the path of the project root.
+script_to_run - path (relative to the __default__ cell link) for the script to run
+resources - TODO(cjhopman): Figure out what these two resources mean and why they both exist
+default_runtime_resources - TODO(cjhopman): Figure out what these two resources mean and why they both exist
+cell_names - list of cell names (including a __default__ for this script's own cell)
+cell_paths - list of paths corresponding to the names in cell_names
+
+!><\\>
 #!/bin/bash
 # Run with -e so the script will fail if any of the steps fail.
 set -e
@@ -94,6 +108,13 @@ trap "rm -rf $BUCK_TMP_ROOT" EXIT HUP INT TERM
   <cell_paths:{x|   <x><\n>}>
   )
 
+  # TODO(cjhopman): This is crazy. we are modifying the binary's own output directory here. Why
+  # wouldn't this have just been done in java?
+  # I think this code is creating a symlink for each cell at a path of basically
+  # `dirname <<THIS_SCRIPT>>`/cells/<cell_name>. But there's no reason to do that here, and it
+  # forces us to do really complex stuff to get it correct. The code is hard to follow, so I came
+  # to the conclusion based on observation.
+  # Also, what are these even used for? The path that they are at isn't exported to the user script?
   # The following operation needs to be atomic.
   if [ ! -d "$BUCK_REAL_ROOT/cells/" ] ; then
     tmpfolder="`mktemp -d "$BUCK_REAL_ROOT/tmp.XXXXXXXXXX"`"
@@ -111,11 +132,19 @@ trap "rm -rf $BUCK_TMP_ROOT" EXIT HUP INT TERM
     "__default__/$SCRIPT_TO_RUN"
   )
 
+  # TODO(cjhopman): Why do we do this here? We already create links for these in Java.
+  # This code appears to be creating links for all of our resources in the invocation-specific
+  # directory pointing back at the files through the cell symlinks we created above... Why do we do
+  # this? And how do we expect people to access these paths?
   for path in "${SYMLINK_PATHS[@]}"; do
     mkdir -p "$(dirname "$path")"
     ln -sf "$BUCK_REAL_ROOT/cells/$path" "$BUCK_TMP_ROOT/$path"
   done
 )
 
+# It seems like people can access resources through either BUCK_PROJECT_ROOT, in which case they
+# are accessing invocation-specific links or through BUCK_DEFAULT_RUNTIME_RESOURCES in which case
+# they are accessing the links created in Java. #1, there's no reason to have invocation-specific
+# links. #2 there's also no reason to have multiple structures of these links laid out.
 export BUCK_PROJECT_ROOT="$BUCK_TMP_ROOT/__default__"
 exec "$BUCK_TMP_ROOT/__default__/$SCRIPT_TO_RUN" "$@"