Detect duplicate CacheIds

Fixes wixtoolset/issues#4628

4 files changed, 43 insertions, 11 deletions

diff --git a/src/WixToolset.Core.Burn/Bind/BindBundleCommand.cs b/src/WixToolset.Core.Burn/Bind/BindBundleCommand.cs
index 5ddb6be1..37fc17f9 100644
--- a/src/WixToolset.Core.Burn/Bind/BindBundleCommand.cs
+++ b/src/WixToolset.Core.Burn/Bind/BindBundleCommand.cs
@@ -359,7 +359,6 @@ namespace WixToolset.Core.Burn
                 this.BackendHelper.ResolveDelayedFields(this.DelayedFields, variableCache);
             }
 
-            Dictionary<string, WixDependencyProviderSymbol> dependencySymbolsByKey;
             {
                 var command = new ProcessDependencyProvidersCommand(this.Messaging, section, facades);
                 command.Execute();
@@ -368,7 +367,6 @@ namespace WixToolset.Core.Burn
                 {
                     bundleSymbol.ProviderKey = command.BundleProviderKey; // set the overridable bundle provider key.
                 }
-                dependencySymbolsByKey = command.DependencySymbolsByKey;
             }
 
             // Update the bundle per-machine/per-user scope based on the chained packages.
@@ -381,6 +379,13 @@ namespace WixToolset.Core.Burn
                 command.Execute();
             }
 
+            this.DetectDuplicateCacheIds(facades);
+
+            if (this.Messaging.EncounteredError)
+            {
+                return;
+            }
+
             // Give the extension one last hook before generating the output files.
             foreach (var extension in this.BackendExtensions)
             {
@@ -581,6 +586,24 @@ namespace WixToolset.Core.Burn
             }
         }
 
+        private void DetectDuplicateCacheIds(IDictionary<string, PackageFacade> facades)
+        {
+            var duplicateCacheIdDetector = new Dictionary<string, WixBundlePackageSymbol>();
+
+            foreach (var facade in facades.Values)
+            {
+                if (duplicateCacheIdDetector.TryGetValue(facade.PackageSymbol.CacheId, out var collisionPackage))
+                {
+                    this.Messaging.Write(BurnBackendErrors.DuplicateCacheIds(collisionPackage.SourceLineNumbers, facade.PackageSymbol.CacheId));
+                    this.Messaging.Write(BurnBackendErrors.DuplicateCacheIds2(facade.PackageSymbol.SourceLineNumbers, facade.PackageSymbol.CacheId));
+                }
+                else
+                {
+                    duplicateCacheIdDetector.Add(facade.PackageSymbol.CacheId, facade.PackageSymbol);
+                }
+            }
+        }
+
         private IEnumerable<T> GetRequiredSymbols<T>() where T : IntermediateSymbol
         {
             var symbols = this.Output.Sections.Single().Symbols.OfType<T>().ToList();
diff --git a/src/WixToolset.Core.Burn/BurnBackendErrors.cs b/src/WixToolset.Core.Burn/BurnBackendErrors.cs
index 106c3b31..4c846e8a 100644
--- a/src/WixToolset.Core.Burn/BurnBackendErrors.cs
+++ b/src/WixToolset.Core.Burn/BurnBackendErrors.cs
@@ -6,10 +6,15 @@ namespace WixToolset.Core.Burn
 
     internal static class BurnBackendErrors
     {
-        //public static Message ReplaceThisWithTheFirstError(SourceLineNumber sourceLineNumbers)
-        //{
-        //    return Message(sourceLineNumbers, Ids.ReplaceThisWithTheFirstError, "format string", arg1, arg2);
-        //}
+        public static Message DuplicateCacheIds(SourceLineNumber originalLineNumber, string cacheId)
+        {
+            return Message(originalLineNumber, Ids.DuplicateCacheIds, "The cache id '{0}' has been duplicated as indicated in the following message.", cacheId);
+        }
+
+        public static Message DuplicateCacheIds2(SourceLineNumber duplicateLineNumber, string cacheId)
+        {
+            return Message(duplicateLineNumber, Ids.DuplicateCacheIds2, "Each cache id must be unique. '{0}' has been used before as indicated in the previous message.", cacheId);
+        }
 
         private static Message Message(SourceLineNumber sourceLineNumber, Ids id, string format, params object[] args)
         {
@@ -18,7 +23,8 @@ namespace WixToolset.Core.Burn
 
         public enum Ids
         {
-            // ReplaceThisWithTheFirstError = 8000,
+            DuplicateCacheIds = 8000,
+            DuplicateCacheIds2 = 8001,
         }
     }
 }
diff --git a/src/test/WixToolsetTest.CoreIntegration/BundleFixture.cs b/src/test/WixToolsetTest.CoreIntegration/BundleFixture.cs
index 38554b70..d121da0f 100644
--- a/src/test/WixToolsetTest.CoreIntegration/BundleFixture.cs
+++ b/src/test/WixToolsetTest.CoreIntegration/BundleFixture.cs
@@ -280,7 +280,7 @@ namespace WixToolsetTest.CoreIntegration
             }
         }
 
-        [Fact(Skip = "https://github.com/wixtoolset/issues/issues/4628")]
+        [Fact]
         public void CantBuildWithDuplicateCacheIds()
         {
             var folder = TestData.Get(@"TestData");
@@ -302,7 +302,7 @@ namespace WixToolsetTest.CoreIntegration
                     "-o", exePath,
                 });
 
-                Assert.InRange(result.ExitCode, 2, Int32.MaxValue);
+                Assert.Equal(8001, result.ExitCode);
             }
         }
 
diff --git a/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs b/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs
index 5c58ef50..0c350042 100644
--- a/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs
+++ b/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs
@@ -2,8 +2,11 @@
 <Wix xmlns="http://wixtoolset.org/schemas/v4/wxs">
     <Fragment>
         <PackageGroup Id="BundlePackages">
-            <ExePackage Id="Manual1" SourceFile="burn.exe" Name="manual1\burn.exe" CacheId="Manual" />
-            <ExePackage Id="Manual2" SourceFile="burn.exe" Name="manual2\burn.exe" CacheId="Manual" />
+            <ExePackage Id="Manual1" SourceFile="burn.exe" Name="manual1\burn.exe" DetectCondition="test" CacheId="!(wix.WixVariable1)" />
+            <ExePackage Id="Manual2" SourceFile="burn.exe" Name="manual2\burn.exe" DetectCondition="test" CacheId="!(wix.WixVariable2)" />
         </PackageGroup>
+
+        <WixVariable Id="WixVariable1" Value="CollidingCacheId" />
+        <WixVariable Id="WixVariable2" Value="CollidingCacheId" />
     </Fragment>
 </Wix>