Add documentation for workspace function

--
MOS_MIGRATED_REVID=112971086

4 files changed, 129 insertions, 48 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index 8caec89db0..d48aa7316c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Package.Builder;
 import com.google.devtools.build.lib.packages.Package.LegacyBuilder;
 import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.BuiltinFunction;
@@ -40,11 +41,15 @@ import com.google.devtools.build.lib.syntax.FunctionSignature;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.syntax.Runtime;
+import com.google.devtools.build.lib.syntax.SkylarkList;
+import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor;
 import com.google.devtools.build.lib.vfs.Path;
 
 import java.io.File;
 import java.io.IOException;
 import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import javax.annotation.Nullable;
 
@@ -161,23 +166,38 @@ public class WorkspaceFactory {
     localReporter.clear();
   }
 
-  // TODO(bazel-team): use @SkylarkSignature annotations on a BuiltinFunction.Factory
-  // for signature + documentation of this and other functions in this file.
-  private static BuiltinFunction newWorkspaceNameFunction() {
-    return new BuiltinFunction(
-        "workspace", FunctionSignature.namedOnly("name"), BuiltinFunction.USE_AST_ENV) {
-      public Object invoke(String name, FuncallExpression ast, Environment env)
-          throws EvalException {
-        String errorMessage = LabelValidator.validateTargetName(name);
-        if (errorMessage != null) {
-          throw new EvalException(ast.getLocation(), errorMessage);
+  @SkylarkSignature(name = "workspace", objectType = Object.class, returnType = SkylarkList.class,
+      doc = "Sets the name for this workspace. Workspace names should be a Java-package-style "
+          + "description of the project, using underscores as separators, e.g., "
+          + "github.com/bazelbuild/bazel should use com_github_bazelbuild_bazel. Names must start "
+          + "with a letter and can only contain letters, numbers, and underscores.",
+      mandatoryPositionals = {
+          @SkylarkSignature.Param(name = "name", type = String.class,
+              doc = "the name of the workspace.")},
+      documented = true, useAst = true, useEnvironment = true)
+  private static final BuiltinFunction.Factory newWorkspaceFunction =
+      new BuiltinFunction.Factory("workspace") {
+        public BuiltinFunction create() {
+          return new BuiltinFunction(
+              "workspace", FunctionSignature.namedOnly("name"), BuiltinFunction.USE_AST_ENV) {
+            public Object invoke(String name, FuncallExpression ast, Environment env)
+                throws EvalException {
+              Pattern legalWorkspaceName = Pattern.compile("^\\p{Alpha}\\w*$");
+              Matcher matcher = legalWorkspaceName.matcher(name);
+              if (!matcher.matches()) {
+                throw new EvalException(
+                    ast.getLocation(), name + " is not a legal workspace name");
+              }
+              String errorMessage = LabelValidator.validateTargetName(name);
+              if (errorMessage != null) {
+                throw new EvalException(ast.getLocation(), errorMessage);
+              }
+              PackageFactory.getContext(env, ast).pkgBuilder.setWorkspaceName(name);
+              return NONE;
+            }
+          };
         }
-
-        PackageFactory.getContext(env, ast).pkgBuilder.setWorkspaceName(name);
-        return NONE;
-      }
-    };
-  }
+      };
 
   private static BuiltinFunction newBindFunction(final RuleFactory ruleFactory) {
     return new BuiltinFunction(
@@ -256,7 +276,7 @@ public class WorkspaceFactory {
 
   private void addWorkspaceFunctions(Environment workspaceEnv, StoredEventHandler localReporter) {
     try {
-      workspaceEnv.update("workspace", newWorkspaceNameFunction());
+      workspaceEnv.setup("workspace", newWorkspaceFunction.apply());
       for (Map.Entry<String, BaseFunction> function : workspaceFunctions.entrySet()) {
         workspaceEnv.update(function.getKey(), function.getValue());
       }
@@ -296,4 +316,8 @@ public class WorkspaceFactory {
   public static ClassObject newNativeModule(RuleClassProvider ruleClassProvider) {
     return newNativeModule(createWorkspaceFunctions(ruleClassProvider));
   }
+
+  static {
+    SkylarkSignatureProcessor.configureSkylarkFunctions(WorkspaceFactory.class);
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
index 13be11547d..d2d8b7c78e 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.packages;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
@@ -28,7 +29,6 @@ import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.vfs.Path;
 
-import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -42,38 +42,95 @@ import java.util.List;
 @RunWith(JUnit4.class)
 public class WorkspaceFactoryTest {
 
-  private Scratch scratch;
-  private Path root;
+  @Test
+  public void testLoadError() throws Exception {
+    // WS with a syntax error: '//a' should end with .bzl.
+    WorkspaceFactoryHelper helper = parse("load('//a', 'a')");
+    helper.assertLexingExceptionThrown();
+    assertThat(helper.getLexerError())
+        .contains("The label must reference a file with extension '.bzl'");
+  }
 
-  @Before
-  public void setUpFileSystem() throws Exception {
-    scratch = new Scratch("/");
-    root = scratch.dir("/workspace");
+  @Test
+  public void testWorkspaceName() throws Exception {
+    WorkspaceFactoryHelper helper = parse("workspace(name = 'my_ws')");
+    assertEquals("my_ws", helper.getPackage().getWorkspaceName());
   }
 
   @Test
-  public void testLoadError() throws Exception {
-    // WS with a syntax error: '//a' should end with .bzl.
-    Path workspaceFilePath = scratch.file("/workspace/WORKSPACE", "load('//a', 'a')");
-    LegacyBuilder builder = Package.newExternalPackageBuilder(workspaceFilePath, "");
-    WorkspaceFactory factory =
-        new WorkspaceFactory(
-            builder,
-            TestRuleClassProvider.getRuleClassProvider(),
-            ImmutableList.<PackageFactory.EnvironmentExtension>of(),
-            Mutability.create("test"),
-            root,
-            root);
-    StoredEventHandler localReporter = new StoredEventHandler();
-    try {
-      factory.parse(ParserInputSource.create(workspaceFilePath), localReporter);
-      fail("Parsing " + workspaceFilePath + " should have failed");
-    } catch (IOException e) {
-      assertThat(e.getMessage()).contains("Failed to parse " + workspaceFilePath);
+  public void testWorkspaceStartsWithNumber() throws Exception {
+    WorkspaceFactoryHelper helper = parse("workspace(name = '123abc')");
+    assertThat(helper.getParserError()).contains("123abc is not a legal workspace name");
+  }
+
+  @Test
+  public void testWorkspaceWithIllegalCharacters() throws Exception {
+    WorkspaceFactoryHelper helper = parse("workspace(name = 'a.b.c')");
+    assertThat(helper.getParserError()).contains("a.b.c is not a legal workspace name");
+  }
+
+  private WorkspaceFactoryHelper parse(String... args) {
+    return new WorkspaceFactoryHelper(args);
+  }
+
+  /**
+   * Parses a WORKSPACE file with the given content.
+   */
+  private class WorkspaceFactoryHelper {
+    private final LegacyBuilder builder;
+    private final WorkspaceFactory factory;
+    private final Exception exception;
+    private final ImmutableList<Event> events;
+
+    public WorkspaceFactoryHelper(String... args) {
+      Path root = null;
+      Path workspaceFilePath = null;
+      try {
+        Scratch scratch = new Scratch("/");
+        root = scratch.dir("/workspace");
+        workspaceFilePath = scratch.file("/workspace/WORKSPACE", args);
+      } catch (IOException e) {
+        fail("Shouldn't happen: " + e.getMessage());
+      }
+      StoredEventHandler eventHandler = new StoredEventHandler();
+      builder = Package.newExternalPackageBuilder(workspaceFilePath, "");
+      this.factory = new WorkspaceFactory(
+          builder,
+          TestRuleClassProvider.getRuleClassProvider(),
+          ImmutableList.<PackageFactory.EnvironmentExtension>of(),
+          Mutability.create("test"),
+          root,
+          root);
+      Exception exception = null;
+      try {
+        factory.parse(ParserInputSource.create(workspaceFilePath), eventHandler);
+      } catch (IOException e) {
+        exception = e;
+      } catch (InterruptedException e) {
+        fail("Shouldn't happen: " + e.getMessage());
+      }
+      this.events = eventHandler.getEvents();
+      this.exception = exception;
+    }
+
+    public Package getPackage() {
+      return builder.build();
+    }
+
+    public void assertLexingExceptionThrown() {
+      assertNotNull(exception);
+      assertThat(exception.getMessage()).contains("Failed to parse /workspace/WORKSPACE");
+    }
+
+    public String getLexerError() {
+      assertEquals(1, events.size());
+      return events.get(0).getMessage();
+    }
+
+    public String getParserError() {
+      List<Event> events = builder.getEvents();
+      assertThat(events.size()).isGreaterThan(0);
+      return events.get(0).getMessage();
     }
-    List<Event> events = localReporter.getEvents();
-    assertEquals("Incorrect size for " + events, 1, events.size());
-    assertThat(events.get(0).getMessage())
-        .contains("The label must reference a file with extension '.bzl'");
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java
index 71c3cf296f..3f7c0d56d3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java
@@ -163,7 +163,7 @@ public class WorkspaceFileFunctionTest extends BuildViewTestCase {
         (PackageValue) skyFunc.compute(PackageValue.workspaceKey(workspacePath), getEnv());
     Package pkg = value.getPackage();
     assertTrue(pkg.containsErrors());
-    MoreAsserts.assertContainsEvent(pkg.getEvents(), "target names may not contain '$'");
+    MoreAsserts.assertContainsEvent(pkg.getEvents(), "foo$ is not a legal workspace name");
   }
 
   @Test
diff --git a/src/test/shell/bazel/runfiles_test.sh b/src/test/shell/bazel/runfiles_test.sh
index de9e951fbc..afc2c0d4b5 100755
--- a/src/test/shell/bazel/runfiles_test.sh
+++ b/src/test/shell/bazel/runfiles_test.sh
@@ -25,7 +25,7 @@ source $(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/test-setup.sh \
 # workspace() is specified in the WORKSPACE file.
 function test_runfiles() {
 
-  name=blorp/malorp
+  name=blorp_malorp
   cat > WORKSPACE <<EOF
 workspace(name = "$name")