Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn the reproduction into a test case. #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aiuto
Copy link

@aiuto aiuto commented Aug 17, 2020

  • add a reference to the main repository by name, so that we see we still parse that
  • add another local repository that includes a local reference, so we see we can refer to other repos by name

I think this is the minimal baroque-ness to make myself happy that the proposed change works.

+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java
@@ -14,12 +14,8 @@

 package com.google.devtools.build.lib.rules.repository;

-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
-import com.google.devtools.build.lib.packages.WorkspaceFileValue;
 import com.google.devtools.build.lib.skyframe.RepositoryValue;
-import com.google.devtools.build.lib.vfs.Root;
-import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
@@ -47,37 +43,6 @@ public class RepositoryLoaderFunction implements SkyFunction {
     if (!repository.repositoryExists()) {
       return RepositoryValue.notFound(nameFromRule);
     }
-    RootedPath workspaceFilePath;
-    try {
-      workspaceFilePath =
-          WorkspaceFileHelper.getWorkspaceRootedFile(Root.fromPath(repository.getPath()), env);
-      if (workspaceFilePath == null) {
-        return null;
-      }
-    } catch (IOException e) {
-      throw new RepositoryLoaderFunctionException(
-          new IOException(
-              "Could not determine workspace file (\"WORKSPACE.bazel\" or \"WORKSPACE\"): "
-                  + e.getMessage()),
-          Transience.PERSISTENT);
-    }
-    SkyKey workspaceKey = WorkspaceFileValue.key(workspaceFilePath);
-    WorkspaceFileValue workspacePackage = (WorkspaceFileValue) env.getValue(workspaceKey);
-    if (workspacePackage == null) {
-      return null;
-    }
-
-    try {
-      String workspaceNameStr = workspacePackage.getPackage().getWorkspaceName();
-      if (workspaceNameStr.isEmpty()) {
-        RepositoryName.create("");
-      } else {
-        RepositoryName.create("@" + workspaceNameStr);
-      }
-    } catch (LabelSyntaxException e) {
-      throw new IllegalStateException(e);
-    }
-
     return RepositoryValue.success(nameFromRule, repository

aiuto added 3 commits August 17, 2020 16:50
- add a reference to the main repository by name, so that we see we still parse that
- add another local repository that includes a local reference, so we see we can refer to other repos by name

I think this is the minimal baroque-ness to make myself happy that the
proposed change works.

```
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java
@@ -14,12 +14,8 @@

 package com.google.devtools.build.lib.rules.repository;

-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
-import com.google.devtools.build.lib.packages.WorkspaceFileValue;
 import com.google.devtools.build.lib.skyframe.RepositoryValue;
-import com.google.devtools.build.lib.vfs.Root;
-import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
@@ -47,37 +43,6 @@ public class RepositoryLoaderFunction implements SkyFunction {
     if (!repository.repositoryExists()) {
       return RepositoryValue.notFound(nameFromRule);
     }
-    RootedPath workspaceFilePath;
-    try {
-      workspaceFilePath =
-          WorkspaceFileHelper.getWorkspaceRootedFile(Root.fromPath(repository.getPath()), env);
-      if (workspaceFilePath == null) {
-        return null;
-      }
-    } catch (IOException e) {
-      throw new RepositoryLoaderFunctionException(
-          new IOException(
-              "Could not determine workspace file (\"WORKSPACE.bazel\" or \"WORKSPACE\"): "
-                  + e.getMessage()),
-          Transience.PERSISTENT);
-    }
-    SkyKey workspaceKey = WorkspaceFileValue.key(workspaceFilePath);
-    WorkspaceFileValue workspacePackage = (WorkspaceFileValue) env.getValue(workspaceKey);
-    if (workspacePackage == null) {
-      return null;
-    }
-
-    try {
-      String workspaceNameStr = workspacePackage.getPackage().getWorkspaceName();
-      if (workspaceNameStr.isEmpty()) {
-        RepositoryName.create("");
-      } else {
-        RepositoryName.create("@" + workspaceNameStr);
-      }
-    } catch (LabelSyntaxException e) {
-      throw new IllegalStateException(e);
-    }
-
     return RepositoryValue.success(nameFromRule, repository
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant