Merge pull request #22869 from Falco20019/grpc-fix-collisions

Avoid collisions in cs files generated by Grpc.Tools

src/csharp/Grpc.Tools.Tests/CSharpGeneratorTest.cs

@@ -84,5 +84,15 @@ namespace Grpc.Tools.Tests
             Assert.AreEqual(1, poss.Length);
             Assert.That(poss[0], Is.EqualTo("out/Foo.cs") | Is.EqualTo("out\\Foo.cs"));
         }
+
+        [Test]
+        public void OutputDirPatched()
+        {
+            var item = Utils.MakeItem("sub/foo.proto", "OutputDir", "out");
+            var output = _generator.PatchOutputDirectory(item);
+            var poss = _generator.GetPossibleOutputs(output);
+            Assert.AreEqual(1, poss.Length);
+            Assert.That(poss[0], Is.EqualTo("out/sub/Foo.cs") | Is.EqualTo("out\\sub\\Foo.cs"));
+        }
     };
 }

src/csharp/Grpc.Tools.Tests/DepFileUtilTest.cs

@@ -67,6 +67,34 @@ namespace Grpc.Tools.Tests
             Assert.AreNotEqual(unsame1, unsame2);
         }
 
+        [Test]
+        public void GetOutputDirWithHash_IsSane()
+        {
+            StringAssert.IsMatch(@"^out[\\/][a-f0-9]{16}$",
+                DepFileUtil.GetOutputDirWithHash("out", "foo.proto"));
+            StringAssert.IsMatch(@"^[a-f0-9]{16}$",
+                DepFileUtil.GetOutputDirWithHash("", "foo.proto"));
+        }
+
+        [Test]
+        public void GetOutputDirWithHash_HashesDir()
+        {
+            string PickHash(string fname) => DepFileUtil.GetOutputDirWithHash("", fname);
+
+            string same1 = PickHash("dir1/dir2/foo.proto");
+            string same2 = PickHash("dir1/dir2/proto.foo");
+            string same3 = PickHash("dir1/dir2/proto");
+            string same4 = PickHash("dir1/dir2/.proto");
+            string unsame1 = PickHash("dir2/foo.proto");
+            string unsame2 = PickHash("/dir2/foo.proto");
+
+            Assert.AreEqual(same1, same2);
+            Assert.AreEqual(same1, same3);
+            Assert.AreEqual(same1, same4);
+            Assert.AreNotEqual(same1, unsame1);
+            Assert.AreNotEqual(unsame1, unsame2);
+        }
+
         //////////////////////////////////////////////////////////////////////////
         // Full file reading tests
 

src/csharp/Grpc.Tools/DepFileUtil.cs

@@ -148,31 +148,63 @@ namespace Grpc.Tools
         /// "out/deadbeef12345678_file.protodep"
         /// </returns>
         /// <remarks>
+        /// See <see cref="GetDirectoryHash"/> for notes on directory hash.
+        /// </remarks>
+        public static string GetDepFilenameForProto(string protoDepDir, string proto)
+        {
+            string dirhash = GetDirectoryHash(proto);
+            string filename = Path.GetFileNameWithoutExtension(proto);
+            return Path.Combine(protoDepDir, $"{dirhash}_{filename}.protodep");
+        }
+
+        /// <summary>
+        /// Construct relative output directory with directory hash
+        /// </summary>
+        /// <param name="outputDir">Relative path to the output directory, e. g. "out"</param>
+        /// <param name="proto">Relative path to the proto item, e. g. "foo/file.proto"</param>
+        /// <returns>
+        /// Full relative path to the directory, e. g. "out/deadbeef12345678"
+        /// </returns>
+        /// <remarks>
+        /// See <see cref="GetDirectoryHash"/> for notes on directory hash.
+        /// </remarks>
+        public static string GetOutputDirWithHash(string outputDir, string proto)
+        {
+            string dirhash = GetDirectoryHash(proto);
+            return Path.Combine(outputDir, dirhash);
+        }
+
+        /// <summary>
+        /// Construct the directory hash from a relative file name
+        /// </summary>
+        /// <param name="proto">Relative path to the proto item, e. g. "foo/file.proto"</param>
+        /// <returns>
+        /// Directory hash based on the file name, e. g. "deadbeef12345678"
+        /// </returns>
+        /// <remarks>
         /// Since a project may contain proto files with the same filename but in different
-        /// directories, a unique filename for the dependency file is constructed based on the
-        /// proto file name both name and directory. The directory path can be arbitrary,
-        /// for example, it can be outside of the project, or an absolute path including
-        /// a drive letter, or a UNC network path. A name constructed from such a path by,
-        /// for example, replacing disallowed name characters with an underscore, may well
-        /// be over filesystem's allowed path length, since it will be located under the
-        /// project and solution directories, which are also some level deep from the root.
+        /// directories, a unique directory for the generated files is constructed based on the
+        /// proto file names directory. The directory path can be arbitrary, for example,
+        /// it can be outside of the project, or an absolute path including a drive letter,
+        /// or a UNC network path. A name constructed from such a path by, for example,
+        /// replacing disallowed name characters with an underscore, may well be over
+        /// filesystem's allowed path length, since it will be located under the project
+        /// and solution directories, which are also some level deep from the root.
         /// Instead of creating long and unwieldy names for these proto sources, we cache
-        /// the full path of the name without the filename, and append the filename to it,
-        /// as in e. g. "foo/file.proto" will yield the name "deadbeef12345678_file", where
-        /// "deadbeef12345678" is a presumed hash value of the string "foo/". This allows
-        /// the file names be short, unique (up to a hash collision), and still allowing
-        /// the user to guess their provenance.
+        /// the full path of the name without the filename, as in e. g. "foo/file.proto"
+        /// will yield the name "deadbeef12345678", where that is a presumed hash value
+        /// of the string "foo". This allows the path to be short, unique (up to a hash
+        /// collision), and still allowing the user to guess their provenance.
         /// </remarks>
-        public static string GetDepFilenameForProto(string protoDepDir, string proto)
+        private static string GetDirectoryHash(string proto)
         {
             string dirname = Path.GetDirectoryName(proto);
             if (Platform.IsFsCaseInsensitive)
             {
                 dirname = dirname.ToLowerInvariant();
             }
-            string dirhash = HashString64Hex(dirname);
-            string filename = Path.GetFileNameWithoutExtension(proto);
-            return Path.Combine(protoDepDir, $"{dirhash}_{filename}.protodep");
+
+            return HashString64Hex(dirname);
         }
 
         // Get a 64-bit hash for a directory string. We treat it as if it were

src/csharp/Grpc.Tools/GeneratorServices.cs

@@ -16,7 +16,6 @@
 
 #endregion
 
-using System;
 using System.IO;
 using System.Text;
 using Microsoft.Build.Framework;
@@ -55,7 +54,62 @@ namespace Grpc.Tools
                 && !gsm.EqualNoCase("false");
         }
 
-        public abstract string[] GetPossibleOutputs(ITaskItem proto);
+        // Update OutputDir and GrpcOutputDir for the item and all subsequent
+        // targets using this item. This should only be done if the real
+        // output directories for protoc should be modified.
+        public virtual ITaskItem PatchOutputDirectory(ITaskItem protoItem)
+        {
+            // Nothing to do
+            return protoItem;
+        }
+
+        public abstract string[] GetPossibleOutputs(ITaskItem protoItem);
+
+        // Calculate part of proto path relative to root. Protoc is very picky
+        // about them matching exactly, so can be we. Expect root be exact prefix
+        // to proto, minus some slash normalization.
+        protected static string GetRelativeDir(string root, string proto, TaskLoggingHelper log)
+        {
+            string protoDir = Path.GetDirectoryName(proto);
+            string rootDir = EndWithSlash(Path.GetDirectoryName(EndWithSlash(root)));
+            if (rootDir == s_dotSlash)
+            {
+                // Special case, otherwise we can return "./" instead of "" below!
+                return protoDir;
+            }
+            if (Platform.IsFsCaseInsensitive)
+            {
+                protoDir = protoDir.ToLowerInvariant();
+                rootDir = rootDir.ToLowerInvariant();
+            }
+            protoDir = EndWithSlash(protoDir);
+            if (!protoDir.StartsWith(rootDir))
+            {
+                log.LogWarning("Protobuf item '{0}' has the ProtoRoot metadata '{1}' " +
+                               "which is not prefix to its path. Cannot compute relative path.",
+                    proto, root);
+                return "";
+            }
+            return protoDir.Substring(rootDir.Length);
+        }
+
+        // './' or '.\', normalized per system.
+        protected static string s_dotSlash = "." + Path.DirectorySeparatorChar;
+
+        protected static string EndWithSlash(string str)
+        {
+            if (str == "")
+            {
+                return s_dotSlash;
+            }
+
+            if (str[str.Length - 1] != '\\' && str[str.Length - 1] != '/')
+            {
+                return str + Path.DirectorySeparatorChar;
+            }
+
+            return str;
+        }
     };
 
     // C# generator services.
@@ -63,23 +117,42 @@ namespace Grpc.Tools
     {
         public CSharpGeneratorServices(TaskLoggingHelper log) : base(log) { }
 
+        public override ITaskItem PatchOutputDirectory(ITaskItem protoItem)
+        {
+            var outputItem = new TaskItem(protoItem);
+            string root = outputItem.GetMetadata(Metadata.ProtoRoot);
+            string proto = outputItem.ItemSpec;
+            string relative = GetRelativeDir(root, proto, Log);
+
+            string outdir = outputItem.GetMetadata(Metadata.OutputDir);
+            string pathStem = Path.Combine(outdir, relative);
+            outputItem.SetMetadata(Metadata.OutputDir, pathStem);
+
+            // Override outdir if GrpcOutputDir present, default to proto output.
+            string grpcdir = outputItem.GetMetadata(Metadata.GrpcOutputDir);
+            if (grpcdir != "")
+            {
+                pathStem = Path.Combine(grpcdir, relative);
+            }
+            outputItem.SetMetadata(Metadata.GrpcOutputDir, pathStem);
+            return outputItem;
+        }
+
         public override string[] GetPossibleOutputs(ITaskItem protoItem)
         {
             bool doGrpc = GrpcOutputPossible(protoItem);
             var outputs = new string[doGrpc ? 2 : 1];
-            string basename = Path.GetFileNameWithoutExtension(protoItem.ItemSpec);
-
+            string proto = protoItem.ItemSpec;
+            string basename = Path.GetFileNameWithoutExtension(proto);
             string outdir = protoItem.GetMetadata(Metadata.OutputDir);
             string filename = LowerUnderscoreToUpperCamelProtocWay(basename);
             outputs[0] = Path.Combine(outdir, filename) + ".cs";
 
             if (doGrpc)
             {
-                // Override outdir if kGrpcOutputDir present, default to proto output.
                 string grpcdir = protoItem.GetMetadata(Metadata.GrpcOutputDir);
                 filename = LowerUnderscoreToUpperCamelGrpcWay(basename);
-                outputs[1] = Path.Combine(
-                    grpcdir != "" ? grpcdir : outdir, filename) + "Grpc.cs";
+                outputs[1] = Path.Combine(grpcdir, filename) + "Grpc.cs";
             }
             return outputs;
         }
@@ -142,7 +215,7 @@ namespace Grpc.Tools
             string proto = protoItem.ItemSpec;
             string filename = Path.GetFileNameWithoutExtension(proto);
             // E. g., ("foo/", "foo/bar/x.proto") => "bar"
-            string relative = GetRelativeDir(root, proto);
+            string relative = GetRelativeDir(root, proto, Log);
 
             var outputs = new string[doGrpc ? 4 : 2];
             string outdir = protoItem.GetMetadata(Metadata.OutputDir);
@@ -151,7 +224,7 @@ namespace Grpc.Tools
             outputs[1] = fileStem + ".pb.h";
             if (doGrpc)
             {
-                // Override outdir if kGrpcOutputDir present, default to proto output.
+                // Override outdir if GrpcOutputDir present, default to proto output.
                 outdir = protoItem.GetMetadata(Metadata.GrpcOutputDir);
                 if (outdir != "")
                 {
@@ -162,52 +235,5 @@ namespace Grpc.Tools
             }
             return outputs;
         }
-
-        // Calculate part of proto path relative to root. Protoc is very picky
-        // about them matching exactly, so can be we. Expect root be exact prefix
-        // to proto, minus some slash normalization.
-        string GetRelativeDir(string root, string proto)
-        {
-            string protoDir = Path.GetDirectoryName(proto);
-            string rootDir = EndWithSlash(Path.GetDirectoryName(EndWithSlash(root)));
-            if (rootDir == s_dotSlash)
-            {
-                // Special case, otherwise we can return "./" instead of "" below!
-                return protoDir;
-            }
-            if (Platform.IsFsCaseInsensitive)
-            {
-                protoDir = protoDir.ToLowerInvariant();
-                rootDir = rootDir.ToLowerInvariant();
-            }
-            protoDir = EndWithSlash(protoDir);
-            if (!protoDir.StartsWith(rootDir))
-            {
-                Log.LogWarning("Protobuf item '{0}' has the ProtoRoot metadata '{1}' " +
-                  "which is not prefix to its path. Cannot compute relative path.",
-                  proto, root);
-                return "";
-            }
-            return protoDir.Substring(rootDir.Length);
-        }
-
-        // './' or '.\', normalized per system.
-        static string s_dotSlash = "." + Path.DirectorySeparatorChar;
-
-        static string EndWithSlash(string str)
-        {
-            if (str == "")
-            {
-                return s_dotSlash;
-            }
-            else if (str[str.Length - 1] != '\\' && str[str.Length - 1] != '/')
-            {
-                return str + Path.DirectorySeparatorChar;
-            }
-            else
-            {
-                return str;
-            }
-        }
-    };
+    }
 }

src/csharp/Grpc.Tools/ProtoCompile.cs

@@ -422,7 +422,9 @@ namespace Grpc.Tools
             if (ProtoPath != null)
             {
                 foreach (string path in ProtoPath)
+                {
                     cmd.AddSwitchMaybe("proto_path", TrimEndSlash(path));
+                }
             }
             cmd.AddSwitchMaybe("dependency_out", DependencyOut);
             cmd.AddSwitchMaybe("error_format", "msvs");

src/csharp/Grpc.Tools/ProtoCompilerOutputs.cs

@@ -40,6 +40,14 @@ namespace Grpc.Tools
         [Required]
         public ITaskItem[] Protobuf { get; set; }
 
+        /// <summary>
+        /// All Proto files in the project. A patched copy of all items from
+        /// Protobuf that might contain updated OutputDir and GrpcOutputDir
+        /// attributes.
+        /// </summary>
+        [Output]
+        public ITaskItem[] PatchedProtobuf { get; set; }
+
         /// <summary>
         /// Output items per each potential output. We do not look at existing
         /// cached dependency even if they exist, since file may be refactored,
@@ -68,16 +76,22 @@ namespace Grpc.Tools
             // Get language-specific possible output. The generator expects certain
             // metadata be set on the proto item.
             var possible = new List<ITaskItem>();
+            var patched = new List<ITaskItem>();
             foreach (var proto in Protobuf)
             {
-                var outputs = generator.GetPossibleOutputs(proto);
+                var patchedProto = generator.PatchOutputDirectory(proto);
+                patched.Add(patchedProto);
+
+                var outputs = generator.GetPossibleOutputs(patchedProto);
                 foreach (string output in outputs)
                 {
                     var ti = new TaskItem(output);
-                    ti.SetMetadata(Metadata.Source, proto.ItemSpec);
+                    ti.SetMetadata(Metadata.Source, patchedProto.ItemSpec);
                     possible.Add(ti);
                 }
             }
+
+            PatchedProtobuf = patched.ToArray();
             PossibleOutputs = possible.ToArray();
 
             return !Log.HasLoggedErrors;

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets

@@ -135,6 +135,7 @@
       <!-- Out-of-project files will have respective ProtoRoot='%(RelativeDir)'. -->
       <Protobuf_Rooted Include="@(_Protobuf_NoRootElsewhere)">
         <ProtoRoot>%(RelativeDir)</ProtoRoot>
+        <ProtoRoot Condition=" '$(Protobuf_ProtoRoot)' != '' ">$(Protobuf_ProtoRoot)</ProtoRoot>
       </Protobuf_Rooted>
     </ItemGroup>
   </Target>
@@ -167,7 +168,13 @@
                           Protobuf="@(Protobuf_Compile)"
                           Generator="$(Protobuf_Generator)">
       <Output TaskParameter="PossibleOutputs" ItemName="Protobuf_ExpectedOutputs" />
+      <Output TaskParameter="PatchedProtobuf" ItemName="_PatchedProtobuf" />
     </ProtoCompilerOutputs>
+    <!-- Replace Protobuf_Compile with PatchedProtobuf. -->
+    <ItemGroup>
+       <Protobuf_Compile Remove="@(_PatchedProtobuf)"/>
+       <Protobuf_Compile Include ="@(_PatchedProtobuf)"/>
+    </ItemGroup>
     <!-- Read any dependency files from previous compiles. -->
     <ProtoReadDependencies Condition=" '$(Protobuf_DepFilesPath)' != '' and '$(DisableProtobufDesignTimeBuild)' != 'true' "
                            Protobuf="@(Protobuf_Compile)"