Bundle input protos together with generated/compiled classes (bazel-contrib#949)

* Init

* Pass bazel test //test/... with single _adjust_resources_path

* adjust_resources_path returns string instead of tuple

* remove prefix handling from ScalacProcessor.copyResources

* Remove unused method

* Remove resourceStripPrefix from CompileOptions

* Remove special path handling from ScalacProcessor because it is done in rule_impls.bzl

* Fix macro visibility

* Remove print

* Formatting

* Add comments

* more test cases

* lint

* Rebase on master

* extract resources.bzl

Author: simuons

scala/private/phases/phase_compile.bzl

@@ -3,7 +3,6 @@
 #
 # DOCUMENT THIS
 #
-load("@bazel_skylib//lib:paths.bzl", _paths = "paths")
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
 load("@bazel_tools//tools/jdk:toolchain_utils.bzl", "find_java_runtime_toolchain", "find_java_toolchain")
 load(
@@ -12,10 +11,10 @@ load(
 )
 load(
     "@io_bazel_rules_scala//scala/private:rule_impls.bzl",
-    _adjust_resources_path_by_default_prefixes = "adjust_resources_path_by_default_prefixes",
     _compile_scala = "compile_scala",
     _expand_location = "expand_location",
 )
+load(":resources.bzl", _resource_paths = "paths")
 
 _java_extension = ".java"
 
@@ -492,38 +491,10 @@ def _try_to_compile_java_jar(
         java_compilation_provider = provider,
     )
 
-def _adjust_resources_path(resource, resource_strip_prefix):
-    if resource_strip_prefix:
-        return _adjust_resources_path_by_strip_prefix(resource, resource_strip_prefix)
-    else:
-        return _adjust_resources_path_by_default_prefixes(resource.path)
-
 def _add_resources_cmd(ctx):
-    res_cmd = []
-    for f in ctx.files.resources:
-        c_dir, res_path = _adjust_resources_path(
-            f,
-            ctx.attr.resource_strip_prefix,
-        )
-        target_path = res_path
-        if target_path[0] == "/":
-            target_path = target_path[1:]
-        line = "{target_path}={c_dir}{res_path}\n".format(
-            res_path = res_path,
-            target_path = target_path,
-            c_dir = c_dir,
-        )
-        res_cmd.extend([line])
-    return "".join(res_cmd)
-
-def _adjust_resources_path_by_strip_prefix(resource, resource_strip_prefix):
-    path = resource.path
-    prefix = _paths.join(resource.owner.workspace_root, resource_strip_prefix)
-    if not path.startswith(prefix):
-        fail("Resource file %s is not under the specified prefix %s to strip" % (path, prefix))
-
-    clean_path = path[len(prefix):]
-    return prefix, clean_path
+    paths = _resource_paths(ctx.files.resources, ctx.attr.resource_strip_prefix)
+    lines = ["{target}={source}\n".format(target = p[0], source = p[1]) for p in paths]
+    return "".join(lines)
 
 def _collect_java_providers_of(deps):
     providers = []

scala/private/resources.bzl

@@ -0,0 +1,49 @@
+load("@bazel_skylib//lib:paths.bzl", _paths = "paths")
+
+def paths(resources, resource_strip_prefix):
+    """Return a list of path tuples (target, source) where:
+        target - is a path in the archive (with given prefix stripped off)
+        source - is an absolute path of the resource file
+
+    Tuple ordering is aligned with zipper format ie zip_path=file
+
+    Args:
+        resources: list of file objects
+        resource_strip_prefix: string to strip from resource path
+    """
+    return [(_target_path(resource, resource_strip_prefix), resource.path) for resource in resources]
+
+def _target_path(resource, resource_strip_prefix):
+    path = _target_path_by_strip_prefix(resource, resource_strip_prefix) if resource_strip_prefix else _target_path_by_default_prefixes(resource.path)
+    return _strip_prefix(path, "/")
+
+def _target_path_by_strip_prefix(resource, resource_strip_prefix):
+    # Start from absolute resource path and then strip roots so we get to correct short path
+    # resource.short_path sometimes give weird results ie '../' prefix
+    path = resource.path
+    path = _strip_prefix(path, resource.owner.workspace_root + "/")
+    path = _strip_prefix(path, resource.root.path + "/")
+
+    # proto_library translates strip_import_prefix to proto_source_root which includes root so we have to strip it
+    prefix = _strip_prefix(resource_strip_prefix, resource.root.path + "/")
+    if not path.startswith(prefix):
+        fail("Resource file %s is not under the specified prefix %s to strip" % (path, prefix))
+    return path[len(prefix):]
+
+def _target_path_by_default_prefixes(path):
+    #  Here we are looking to find out the offset of this resource inside
+    #  any resources folder. We want to return the root to the resources folder
+    #  and then the sub path inside it
+    dir_1, dir_2, rel_path = path.partition("resources")
+    if rel_path:
+        return rel_path
+
+    #  The same as the above but just looking for java
+    (dir_1, dir_2, rel_path) = path.partition("java")
+    if rel_path:
+        return rel_path
+
+    return path
+
+def _strip_prefix(path, prefix):
+    return path[len(prefix):] if path.startswith(prefix) else path

scala/private/rule_impls.bzl

@@ -21,21 +21,7 @@ load(
     ":common.bzl",
     _collect_plugin_paths = "collect_plugin_paths",
 )
-
-def adjust_resources_path_by_default_prefixes(path):
-    #  Here we are looking to find out the offset of this resource inside
-    #  any resources folder. We want to return the root to the resources folder
-    #  and then the sub path inside it
-    dir_1, dir_2, rel_path = path.partition("resources")
-    if rel_path:
-        return dir_1 + dir_2, rel_path
-
-    #  The same as the above but just looking for java
-    (dir_1, dir_2, rel_path) = path.partition("java")
-    if rel_path:
-        return dir_1 + dir_2, rel_path
-
-    return "", path
+load(":resources.bzl", _resource_paths = "paths")
 
 def expand_location(ctx, flags):
     if hasattr(ctx.attr, "data"):
@@ -151,6 +137,7 @@ CurrentTarget: {current_target}
 
     toolchain = ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"]
     scalacopts = [ctx.expand_location(v, input_plugins) for v in toolchain.scalacopts + in_scalacopts]
+    resource_paths = _resource_paths(resources, resource_strip_prefix)
 
     scalac_args = """
 Classpath: {cp}
@@ -161,11 +148,9 @@ Manifest: {manifest}
 Plugins: {plugin_arg}
 PrintCompileTime: {print_compile_time}
 ExpectJavaOutput: {expect_java_output}
-ResourceDests: {resource_dest}
+ResourceTargets: {resource_targets}
+ResourceSources: {resource_sources}
 ResourceJars: {resource_jars}
-ResourceSrcs: {resource_src}
-ResourceShortPaths: {resource_short_paths}
-ResourceStripPrefix: {resource_strip_prefix}
 ScalacOpts: {scala_opts}
 SourceJars: {srcjars}
 DependencyAnalyzerMode: {dependency_analyzer_mode}
@@ -184,13 +169,8 @@ DiagnosticsFile: {diagnostics_output}
         files = _join_path(sources),
         srcjars = _join_path(all_srcjars.to_list()),
         # the resource paths need to be aligned in order
-        resource_src = ",".join([f.path for f in resources]),
-        resource_short_paths = ",".join([f.short_path for f in resources]),
-        resource_dest = ",".join([
-            adjust_resources_path_by_default_prefixes(f.short_path)[1]
-            for f in resources
-        ]),
-        resource_strip_prefix = resource_strip_prefix,
+        resource_targets = ",".join([p[0] for p in resource_paths]),
+        resource_sources = ",".join([p[1] for p in resource_paths]),
         resource_jars = _join_path(resource_jars),
         dependency_analyzer_mode = dependency_analyzer_mode,
         unused_dependency_checker_mode = unused_dependency_checker_mode,

scala_proto/private/scalapb_aspect.bzl

@@ -51,7 +51,9 @@ def _compile_scala(
         output,
         scalapb_jar,
         deps_java_info,
-        implicit_deps):
+        implicit_deps,
+        resources,
+        resource_strip_prefix):
     manifest = ctx.actions.declare_file(
         label.name + "_MANIFEST.MF",
         sibling = scalapb_jar,
@@ -78,8 +80,8 @@ def _compile_scala(
         all_srcjars = depset([scalapb_jar]),
         transitive_compile_jars = merged_deps.transitive_compile_time_jars,
         plugins = [],
-        resource_strip_prefix = "",
-        resources = [],
+        resource_strip_prefix = resource_strip_prefix,
+        resources = resources,
         resource_jars = [],
         labels = {},
         in_scalacopts = [],
@@ -193,6 +195,8 @@ def _scalapb_aspect_impl(target, ctx):
                 scalapb_file,
                 deps,
                 imps,
+                compile_protos,
+                "" if target_ti.proto_source_root == "." else target_ti.proto_source_root,
             )
         else:
             # this target is only an aggregation target

src/java/io/bazel/rulesscala/scalac/CompileOptions.java

@@ -1,6 +1,6 @@
 package io.bazel.rulesscala.scalac;
 
-
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -16,8 +16,7 @@ public class CompileOptions {
   public final String[] files;
   public final String[] sourceJars;
   public final String[] javaFiles;
-  public final Map<String, Resource> resourceFiles;
-  public final String resourceStripPrefix;
+  public final List<Resource> resourceFiles;
   public final String[] resourceJars;
   public final String[] classpathResourceFiles;
   public final String[] directJars;
@@ -52,7 +51,6 @@ public CompileOptions(List<String> args) {
 
     sourceJars = getCommaList(argMap, "SourceJars");
     resourceFiles = getResources(argMap);
-    resourceStripPrefix = getOrEmpty(argMap, "ResourceStripPrefix");
     resourceJars = getCommaList(argMap, "ResourceJars");
     classpathResourceFiles = getCommaList(argMap, "ClasspathResourceSrcs");
 
@@ -70,29 +68,21 @@ public CompileOptions(List<String> args) {
     diagnosticsFile = getOrElse(argMap, "DiagnosticsFile", null);
   }
 
-  private static Map<String, Resource> getResources(Map<String, String> args) {
-    String[] keys = getCommaList(args, "ResourceSrcs");
-    String[] dests = getCommaList(args, "ResourceDests");
-    String[] shortPaths = getCommaList(args, "ResourceShortPaths");
-
-    if (keys.length != dests.length)
-      throw new RuntimeException(
-          String.format(
-              "mismatch in resources: keys: %s dests: %s",
-              getOrEmpty(args, "ResourceSrcs"), getOrEmpty(args, "ResourceDests")));
+  private static List<Resource> getResources(Map<String, String> args) {
+    String[] targets = getCommaList(args, "ResourceTargets");
+    String[] sources = getCommaList(args, "ResourceSources");
 
-    if (keys.length != shortPaths.length)
+    if (targets.length != sources.length)
       throw new RuntimeException(
           String.format(
-              "mismatch in resources: keys: %s shortPaths: %s",
-              getOrEmpty(args, "ResourceSrcs"), getOrEmpty(args, "ResourceShortPaths")));
+              "mismatch in resources: targets: %s sources: %s",
+              getOrEmpty(args, "ResourceTargets"), getOrEmpty(args, "ResourceSources")));
 
-    HashMap<String, Resource> res = new HashMap();
-    for (int idx = 0; idx < keys.length; idx++) {
-      Resource resource = new Resource(dests[idx], shortPaths[idx]);
-      res.put(keys[idx], resource);
+    List<Resource> resources = new ArrayList<Resource>();
+    for (int idx = 0; idx < targets.length; idx++) {
+      resources.add(new Resource(targets[idx], sources[idx]));
     }
-    return res;
+    return resources;
   }
 
   private static HashMap<String, String> buildArgMap(List<String> lines) {

src/java/io/bazel/rulesscala/scalac/Resource.java

@@ -1,11 +1,11 @@
 package io.bazel.rulesscala.scalac;
 
 public class Resource {
-  public final String destination;
-  public final String shortPath;
+  public final String target;
+  public final String source;
 
-  public Resource(String destination, String shortPath) {
-    this.destination = destination;
-    this.shortPath = shortPath;
+  public Resource(String target, String source) {
+    this.target = target;
+    this.source = source;
   }
 }

src/java/io/bazel/rulesscala/scalac/ScalacProcessor.java

@@ -74,7 +74,7 @@ public void processRequest(List<String> args) throws Exception {
       }
 
       /** Copy the resources */
-      copyResources(ops.resourceFiles, ops.resourceStripPrefix, tmpPath);
+      copyResources(ops.resourceFiles, tmpPath);
 
       /** Extract and copy resources from resource jars */
       copyResourceJars(ops.resourceJars, tmpPath);
@@ -313,44 +313,11 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc)
     }
   }
 
-  private static void copyResources(
-      Map<String, Resource> resources, String resourceStripPrefix, Path dest) throws IOException {
-    for (Entry<String, Resource> e : resources.entrySet()) {
-      Path source = Paths.get(e.getKey());
-      Resource resource = e.getValue();
-      Path shortPath = Paths.get(resource.shortPath);
-      String dstr;
-      // Check if we need to modify resource destination path
-      if (!"".equals(resourceStripPrefix)) {
-        /**
-         * NOTE: We are not using the Resource Hash Value as the destination path when
-         * `resource_strip_prefix` present. The path in the hash value is computed by the
-         * `_adjust_resources_path` in `scala.bzl`. These are the default paths, ie, path that are
-         * automatically computed when there is no `resource_strip_prefix` present. But when
-         * `resource_strip_prefix` is present, we need to strip the prefix from the Source Path and
-         * use that as the new destination path Refer Bazel -> BazelJavaRuleClasses.java#L227 for
-         * details
-         */
-        dstr = getResourcePath(shortPath, resourceStripPrefix);
-      } else {
-        dstr = resource.destination;
-      }
-
-      if (dstr.charAt(0) == '/') {
-        // we don't want to copy to an absolute destination
-        dstr = dstr.substring(1);
-      }
-      if (dstr.startsWith("../")) {
-        // paths to external repositories, for some reason, start with a leading ../
-        // we don't want to copy the resource out of our temporary directory, so
-        // instead we replace ../ with external/
-        // since "external" is a bit of reserved directory in bazel for these kinds
-        // of purposes, we don't expect a collision in the paths.
-        dstr = "external" + dstr.substring(2);
-      }
-      Path target = dest.resolve(dstr);
-      File tfile = target.getParent().toFile();
-      tfile.mkdirs();
+  private static void copyResources(List<Resource> resources, Path dest) throws IOException {
+    for (Resource r : resources) {
+      Path source = Paths.get(r.source);
+      Path target = dest.resolve(r.target);
+      target.getParent().toFile().mkdirs();
       Files.copy(source, target);
     }
   }
@@ -373,24 +340,6 @@ private static void copyClasspathResourcesToRoot(String[] classpathResourceFiles
     }
   }
 
-  private static String getResourcePath(Path source, String resourceStripPrefix)
-      throws RuntimeException {
-    String sourcePath = source.toString();
-    // convert strip prefix to a Path first and back to handle different file systems
-    String resourceStripPrefixPath = Paths.get(resourceStripPrefix).toString();
-    // check if the Resource file is under the specified prefix to strip
-    if (!sourcePath.startsWith(resourceStripPrefixPath)) {
-      // Resource File is not under the specified prefix to strip
-      throw new RuntimeException(
-          "Resource File "
-              + sourcePath
-              + " is not under the specified strip prefix "
-              + resourceStripPrefix);
-    }
-    String newResPath = sourcePath.substring(resourceStripPrefix.length());
-    return newResPath;
-  }
-
   private static void copyResourceJars(String[] resourceJars, Path dest) throws IOException {
     for (String jarPath : resourceJars) {
       extractJar(jarPath, dest.toString(), null);

src/rules_scala1.iml

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<module type="JAVA_MODULE" version="4">
+  <component name="NewModuleRootManager" inherit-compiler-output="true">
+    <exclude-output />
+    <content url="file://$MODULE_DIR$">
+      <sourceFolder url="file://$MODULE_DIR$/java" isTestSource="false" />
+      <sourceFolder url="file://$MODULE_DIR$/scala" isTestSource="false" />
+    </content>
+    <orderEntry type="inheritedJdk" />
+    <orderEntry type="sourceFolder" forTests="false" />
+  </component>
+</module>