From 678c92c50c6fb7aa9a093f0d74d4f92742abd5e8 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Mon, 15 Jun 2020 16:07:45 -0700 Subject: Reorganize media assignment to correctly place facade order optimization --- .../Bind/AssignMediaCommand.cs | 159 ++++++++++----------- .../Bind/BindDatabaseCommand.cs | 32 ++--- .../Bind/CabinetBuilder.cs | 14 +- .../Bind/CreateCabinetsCommand.cs | 1 - .../Bind/OptimizeFileFacadesOrderCommand.cs | 38 +++++ .../Bind/UpdateMediaSequencesCommand.cs | 15 +- .../WixToolsetTest.CoreIntegration/CabFixture.cs | 71 +++++++++ .../MultiFileCompressed/PackageComponents.wxs | 4 +- 8 files changed, 208 insertions(+), 126 deletions(-) create mode 100644 src/WixToolset.Core.WindowsInstaller/Bind/OptimizeFileFacadesOrderCommand.cs create mode 100644 src/test/WixToolsetTest.CoreIntegration/CabFixture.cs (limited to 'src') diff --git a/src/WixToolset.Core.WindowsInstaller/Bind/AssignMediaCommand.cs b/src/WixToolset.Core.WindowsInstaller/Bind/AssignMediaCommand.cs index 1d677a70..b75956b4 100644 --- a/src/WixToolset.Core.WindowsInstaller/Bind/AssignMediaCommand.cs +++ b/src/WixToolset.Core.WindowsInstaller/Bind/AssignMediaCommand.cs @@ -35,18 +35,13 @@ namespace WixToolset.Core.WindowsInstaller.Bind private bool FilesCompressed { get; } - public string CabinetNameTemplate { private get; set; } + private string CabinetNameTemplate { get; set; } /// /// Gets cabinets with their file rows. /// public Dictionary> FileFacadesByCabinetMedia { get; private set; } - /// - /// Get media rows. - /// - public Dictionary MediaRows { get; private set; } - /// /// Get uncompressed file rows. This will contain file rows of File elements that are marked with compression=no. /// This contains all the files when Package element is marked with compression=no @@ -55,17 +50,11 @@ namespace WixToolset.Core.WindowsInstaller.Bind public void Execute() { - var filesByCabinetMedia = new Dictionary>(); - - var mediaRows = new Dictionary(); - - var uncompressedFiles = new List(); - - var mediaTable = this.Section.Tuples.OfType().ToList(); - var mediaTemplateTable = this.Section.Tuples.OfType().ToList(); + var mediaTuples = this.Section.Tuples.OfType().ToList(); + var mediaTemplateTuples = this.Section.Tuples.OfType().ToList(); - // If both tables are authored, it is an error. - if (mediaTemplateTable.Count > 0 && mediaTable.Count > 1) + // If both tuples are authored, it is an error. + if (mediaTemplateTuples.Count > 0 && mediaTuples.Count > 1) { throw new WixException(ErrorMessages.MediaTableCollision(null)); } @@ -78,34 +67,44 @@ namespace WixToolset.Core.WindowsInstaller.Bind Cabinet = "#MergeModule.CABinet", }); - filesByCabinetMedia.Add(mergeModuleMediaTuple, new List(this.FileFacades)); + this.FileFacadesByCabinetMedia = new Dictionary> + { + { mergeModuleMediaTuple, this.FileFacades } + }; + + this.UncompressedFileFacades = Array.Empty(); } - else if (mediaTemplateTable.Count == 0) + else if (mediaTemplateTuples.Count == 0) { - this.ManuallyAssignFiles(mediaTable, this.FileFacades, filesByCabinetMedia, mediaRows, uncompressedFiles); + var filesByCabinetMedia = new Dictionary>(); + + var uncompressedFiles = new List(); + + this.ManuallyAssignFiles(mediaTuples, filesByCabinetMedia, uncompressedFiles); + + this.FileFacadesByCabinetMedia = filesByCabinetMedia.ToDictionary(kvp => kvp.Key, kvp => (IEnumerable)kvp.Value); + + this.UncompressedFileFacades = uncompressedFiles; } else { - this.AutoAssignFiles(mediaTable, filesByCabinetMedia, mediaRows, uncompressedFiles); - } + var filesByCabinetMedia = new Dictionary>(); - this.FileFacadesByCabinetMedia = new Dictionary>(); + var uncompressedFiles = new List(); - foreach (var mediaRowWithFiles in filesByCabinetMedia) - { - this.FileFacadesByCabinetMedia.Add(mediaRowWithFiles.Key, mediaRowWithFiles.Value); - } + this.AutoAssignFiles(mediaTuples, filesByCabinetMedia, uncompressedFiles); - this.MediaRows = mediaRows; + this.FileFacadesByCabinetMedia = filesByCabinetMedia.ToDictionary(kvp => kvp.Key, kvp => (IEnumerable)kvp.Value); - this.UncompressedFileFacades = uncompressedFiles; + this.UncompressedFileFacades = uncompressedFiles; + } } /// /// Assign files to cabinets based on MediaTemplate authoring. /// /// FileRowCollection - private void AutoAssignFiles(List mediaTable, Dictionary> filesByCabinetMedia, Dictionary mediaRows, List uncompressedFiles) + private void AutoAssignFiles(List mediaTable, Dictionary> filesByCabinetMedia, List uncompressedFiles) { const int MaxCabIndex = 999; @@ -158,6 +157,8 @@ namespace WixToolset.Core.WindowsInstaller.Bind throw new WixException(ErrorMessages.MaximumUncompressedMediaSizeTooLarge(null, maxPreCabSizeInMB)); } + var mediaTuplesByDiskId = new Dictionary(); + foreach (var facade in this.FileFacades) { // When building a product, if the current file is not to be compressed or if @@ -171,44 +172,38 @@ namespace WixToolset.Core.WindowsInstaller.Bind if (currentCabIndex == MaxCabIndex) { // Associate current file with last cab (irrespective of the size) and cab index is not incremented anymore. - var cabinetFiles = filesByCabinetMedia[currentMediaRow]; - facade.DiskId = currentCabIndex; - cabinetFiles.Add(facade); - continue; - } - - // Update current cab size. - currentPreCabSize += (ulong)facade.FileSize; - - if (currentPreCabSize > maxPreCabSizeInBytes) - { - // Overflow due to current file - currentMediaRow = this.AddMediaRow(mediaTemplateRow, ++currentCabIndex); - mediaRows.Add(currentMediaRow.DiskId, currentMediaRow); - filesByCabinetMedia.Add(currentMediaRow, new List()); - - var cabinetFileRows = filesByCabinetMedia[currentMediaRow]; - facade.DiskId = currentCabIndex; - cabinetFileRows.Add(facade); - // Now files larger than MaxUncompressedMediaSize will be the only file in its cabinet so as to respect MaxUncompressedMediaSize - currentPreCabSize = (ulong)facade.FileSize; } else { - // File fits in the current cab. - if (currentMediaRow == null) + // Update current cab size. + currentPreCabSize += (ulong)facade.FileSize; + + // Overflow due to current file + if (currentPreCabSize > maxPreCabSizeInBytes) { - // Create new cab and MediaRow - currentMediaRow = this.AddMediaRow(mediaTemplateRow, ++currentCabIndex); - mediaRows.Add(currentMediaRow.DiskId, currentMediaRow); + currentMediaRow = this.AddMediaTuple(mediaTemplateRow, ++currentCabIndex); + mediaTuplesByDiskId.Add(currentMediaRow.DiskId, currentMediaRow); filesByCabinetMedia.Add(currentMediaRow, new List()); - } - // Associate current file with current cab. - var cabinetFiles = filesByCabinetMedia[currentMediaRow]; - facade.DiskId = currentCabIndex; - cabinetFiles.Add(facade); + // Now files larger than MaxUncompressedMediaSize will be the only file in its cabinet so as to respect MaxUncompressedMediaSize + currentPreCabSize = (ulong)facade.FileSize; + } + else // file fits in the current cab. + { + if (currentMediaRow == null) + { + // Create new cab and MediaRow + currentMediaRow = this.AddMediaTuple(mediaTemplateRow, ++currentCabIndex); + mediaTuplesByDiskId.Add(currentMediaRow.DiskId, currentMediaRow); + filesByCabinetMedia.Add(currentMediaRow, new List()); + } + } } + + // Associate current file with current cab. + var cabinetFiles = filesByCabinetMedia[currentMediaRow]; + facade.DiskId = currentCabIndex; + cabinetFiles.Add(facade); } // If there are uncompressed files and no MediaRow, create a default one. @@ -219,51 +214,45 @@ namespace WixToolset.Core.WindowsInstaller.Bind DiskId = 1, }); - mediaRows.Add(1, defaultMediaRow); + mediaTuplesByDiskId.Add(1, defaultMediaRow); } } /// /// Assign files to cabinets based on Media authoring. /// - /// - /// - private void ManuallyAssignFiles(List mediaTable, IEnumerable fileFacades, Dictionary> filesByCabinetMedia, Dictionary mediaRows, List uncompressedFiles) + private void ManuallyAssignFiles(List mediaTuples, Dictionary> filesByCabinetMedia, List uncompressedFiles) { - if (mediaTable.Any()) + var mediaTuplesByDiskId = new Dictionary(); + + if (mediaTuples.Any()) { - var cabinetMediaRows = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var mediaRow in mediaTable) + var cabinetMediaTuples = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var mediaTuple in mediaTuples) { // If the Media row has a cabinet, make sure it is unique across all Media rows. - if (!String.IsNullOrEmpty(mediaRow.Cabinet)) + if (!String.IsNullOrEmpty(mediaTuple.Cabinet)) { - if (cabinetMediaRows.TryGetValue(mediaRow.Cabinet, out var existingRow)) + if (cabinetMediaTuples.TryGetValue(mediaTuple.Cabinet, out var existingRow)) { - this.Messaging.Write(ErrorMessages.DuplicateCabinetName(mediaRow.SourceLineNumbers, mediaRow.Cabinet)); + this.Messaging.Write(ErrorMessages.DuplicateCabinetName(mediaTuple.SourceLineNumbers, mediaTuple.Cabinet)); this.Messaging.Write(ErrorMessages.DuplicateCabinetName2(existingRow.SourceLineNumbers, existingRow.Cabinet)); } else { - cabinetMediaRows.Add(mediaRow.Cabinet, mediaRow); + cabinetMediaTuples.Add(mediaTuple.Cabinet, mediaTuple); } - } - mediaRows.Add(mediaRow.DiskId, mediaRow); - } - } + filesByCabinetMedia.Add(mediaTuple, new List()); + } - foreach (var mediaRow in mediaRows.Values) - { - if (null != mediaRow.Cabinet) - { - filesByCabinetMedia.Add(mediaRow, new List()); + mediaTuplesByDiskId.Add(mediaTuple.DiskId, mediaTuple); } } - foreach (var facade in fileFacades) + foreach (var facade in this.FileFacades) { - if (!mediaRows.TryGetValue(facade.DiskId, out var mediaRow)) + if (!mediaTuplesByDiskId.TryGetValue(facade.DiskId, out var mediaTuple)) { this.Messaging.Write(ErrorMessages.MissingMedia(facade.SourceLineNumber, facade.DiskId)); continue; @@ -279,7 +268,7 @@ namespace WixToolset.Core.WindowsInstaller.Bind } else // file is marked compressed. { - if (filesByCabinetMedia.TryGetValue(mediaRow, out var cabinetFiles)) + if (filesByCabinetMedia.TryGetValue(mediaTuple, out var cabinetFiles)) { cabinetFiles.Add(facade); } @@ -292,12 +281,12 @@ namespace WixToolset.Core.WindowsInstaller.Bind } /// - /// Adds a row to the media table with cab name template filled in. + /// Adds a tuple to the section with cab name template filled in. /// /// /// /// - private MediaTuple AddMediaRow(WixMediaTemplateTuple mediaTemplateTuple, int cabIndex) + private MediaTuple AddMediaTuple(WixMediaTemplateTuple mediaTemplateTuple, int cabIndex) { return this.Section.AddTuple(new MediaTuple(mediaTemplateTuple.SourceLineNumbers, new Identifier(AccessModifier.Private, cabIndex)) { diff --git a/src/WixToolset.Core.WindowsInstaller/Bind/BindDatabaseCommand.cs b/src/WixToolset.Core.WindowsInstaller/Bind/BindDatabaseCommand.cs index 6c2968ec..da92be69 100644 --- a/src/WixToolset.Core.WindowsInstaller/Bind/BindDatabaseCommand.cs +++ b/src/WixToolset.Core.WindowsInstaller/Bind/BindDatabaseCommand.cs @@ -281,19 +281,6 @@ namespace WixToolset.Core.WindowsInstaller.Bind command.Execute(); } - // Assign files to media. - Dictionary assignedMediaRows; - Dictionary> filesByCabinetMedia; - IEnumerable uncompressedFiles; - { - var command = new AssignMediaCommand(section, this.Messaging, fileFacades, compressed); - command.Execute(); - - assignedMediaRows = command.MediaRows; - filesByCabinetMedia = command.FileFacadesByCabinetMedia; - uncompressedFiles = command.UncompressedFileFacades; - } - // stop processing if an error previously occurred if (this.Messaging.EncounteredError) { @@ -366,10 +353,23 @@ namespace WixToolset.Core.WindowsInstaller.Bind command.Execute(); } - // Update file sequence. + // Assign files to media and update file sequences. + Dictionary> filesByCabinetMedia; + IEnumerable uncompressedFiles; { - var command = new UpdateMediaSequencesCommand(section, fileFacades); - command.Execute(); + var order = new OptimizeFileFacadesOrderCommand(fileFacades); + order.Execute(); + + fileFacades = order.FileFacades; + + var assign = new AssignMediaCommand(section, this.Messaging, fileFacades, compressed); + assign.Execute(); + + filesByCabinetMedia = assign.FileFacadesByCabinetMedia; + uncompressedFiles = assign.UncompressedFileFacades; + + var update = new UpdateMediaSequencesCommand(section, fileFacades); + update.Execute(); } // stop processing if an error previously occurred diff --git a/src/WixToolset.Core.WindowsInstaller/Bind/CabinetBuilder.cs b/src/WixToolset.Core.WindowsInstaller/Bind/CabinetBuilder.cs index 486ee67a..dce89f78 100644 --- a/src/WixToolset.Core.WindowsInstaller/Bind/CabinetBuilder.cs +++ b/src/WixToolset.Core.WindowsInstaller/Bind/CabinetBuilder.cs @@ -7,7 +7,6 @@ namespace WixToolset.Core.WindowsInstaller.Bind using System.IO; using System.Linq; using System.Threading; - using WixToolset.Core.Bind; using WixToolset.Core.Native; using WixToolset.Data; using WixToolset.Extensibility.Services; @@ -18,12 +17,13 @@ namespace WixToolset.Core.WindowsInstaller.Bind /// internal sealed class CabinetBuilder { - private Queue cabinetWorkItems; - private object lockObject; + private readonly object lockObject = new object(); + + private readonly Queue cabinetWorkItems; private int threadCount; // Address of Binder's callback function for Cabinet Splitting - private IntPtr newCabNamesCallBackAddress; + private readonly IntPtr newCabNamesCallBackAddress; /// /// Instantiate a new CabinetBuilder. @@ -38,7 +38,6 @@ namespace WixToolset.Core.WindowsInstaller.Bind } this.cabinetWorkItems = new Queue(); - this.lockObject = new object(); this.Messaging = messaging; this.threadCount = threadCount; @@ -56,10 +55,7 @@ namespace WixToolset.Core.WindowsInstaller.Bind /// Enqueues a CabinetWorkItem to the queue. /// /// cabinet work item - public void Enqueue(CabinetWorkItem cabinetWorkItem) - { - this.cabinetWorkItems.Enqueue(cabinetWorkItem); - } + public void Enqueue(CabinetWorkItem cabinetWorkItem) => this.cabinetWorkItems.Enqueue(cabinetWorkItem); /// /// Create the queued cabinets. diff --git a/src/WixToolset.Core.WindowsInstaller/Bind/CreateCabinetsCommand.cs b/src/WixToolset.Core.WindowsInstaller/Bind/CreateCabinetsCommand.cs index de357e53..9741fcd9 100644 --- a/src/WixToolset.Core.WindowsInstaller/Bind/CreateCabinetsCommand.cs +++ b/src/WixToolset.Core.WindowsInstaller/Bind/CreateCabinetsCommand.cs @@ -171,7 +171,6 @@ namespace WixToolset.Core.WindowsInstaller.Bind return cabbingThreadCount; } - /// /// Creates a work item to create a cabinet. /// diff --git a/src/WixToolset.Core.WindowsInstaller/Bind/OptimizeFileFacadesOrderCommand.cs b/src/WixToolset.Core.WindowsInstaller/Bind/OptimizeFileFacadesOrderCommand.cs new file mode 100644 index 00000000..6943d345 --- /dev/null +++ b/src/WixToolset.Core.WindowsInstaller/Bind/OptimizeFileFacadesOrderCommand.cs @@ -0,0 +1,38 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information. + +namespace WixToolset.Core.WindowsInstaller.Bind +{ + using System; + using System.Collections.Generic; + using WixToolset.Core.Bind; + + internal class OptimizeFileFacadesOrderCommand + { + public OptimizeFileFacadesOrderCommand(List fileFacades) + { + this.FileFacades = fileFacades; + } + + public List FileFacades { get; private set; } + + public List Execute() + { + this.FileFacades.Sort(FileFacadeOptimizer.Instance); + + return this.FileFacades; + } + + private class FileFacadeOptimizer : IComparer + { + public static readonly FileFacadeOptimizer Instance = new FileFacadeOptimizer(); + + public int Compare(FileFacade x, FileFacade y) + { + // TODO: Sort these facades even smarter by directory path and component id + // and maybe file size or file extension and other creative ideas to + // get optimal install speed out of MSI. + return String.Compare(x.ComponentRef, y.ComponentRef, StringComparison.Ordinal); + } + } + } +} diff --git a/src/WixToolset.Core.WindowsInstaller/Bind/UpdateMediaSequencesCommand.cs b/src/WixToolset.Core.WindowsInstaller/Bind/UpdateMediaSequencesCommand.cs index 9aab7b98..bf28b279 100644 --- a/src/WixToolset.Core.WindowsInstaller/Bind/UpdateMediaSequencesCommand.cs +++ b/src/WixToolset.Core.WindowsInstaller/Bind/UpdateMediaSequencesCommand.cs @@ -29,9 +29,7 @@ namespace WixToolset.Core.WindowsInstaller.Bind { var lastSequence = 0; - // Order by Component to group the files by directory. - var optimized = this.OptimizedFileFacades(); - foreach (var facade in optimized) + foreach (var facade in this.FileFacades) { facade.Sequence = ++lastSequence; } @@ -43,8 +41,7 @@ namespace WixToolset.Core.WindowsInstaller.Bind var patchGroups = new Dictionary>(); // sequence the non-patch-added files - var optimized = this.OptimizedFileFacades(); - foreach (var facade in optimized) + foreach (var facade in this.FileFacades) { if (null == mediaTuple) { @@ -108,13 +105,5 @@ namespace WixToolset.Core.WindowsInstaller.Bind } } } - - private IEnumerable OptimizedFileFacades() - { - // TODO: Sort these facades even smarter by directory path and component id - // and maybe file size or file extension and other creative ideas to - // get optimal install speed out of MSI. - return this.FileFacades.OrderBy(f => f.ComponentRef); - } } } diff --git a/src/test/WixToolsetTest.CoreIntegration/CabFixture.cs b/src/test/WixToolsetTest.CoreIntegration/CabFixture.cs new file mode 100644 index 00000000..79471554 --- /dev/null +++ b/src/test/WixToolsetTest.CoreIntegration/CabFixture.cs @@ -0,0 +1,71 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information. + +namespace WixToolsetTest.CoreIntegration +{ + using System; + using System.IO; + using System.Linq; + using WixBuildTools.TestSupport; + using WixToolset.Core.TestPackage; + using Xunit; + + public class CabFixture + { + [Fact] + public void CabinetFilesSequencedCorrectly() + { + var folder = TestData.Get(@"TestData\MultiFileCompressed"); + + using (var fs = new DisposableFileSystem()) + { + var baseFolder = fs.GetFolder(); + var intermediateFolder = Path.Combine(baseFolder, "obj"); + var msiPath = Path.Combine(baseFolder, @"bin\test.msi"); + var cabPath = Path.Combine(baseFolder, @"bin\cab1.cab"); + + var result = WixRunner.Execute(new[] + { + "build", + Path.Combine(folder, "Package.wxs"), + Path.Combine(folder, "PackageComponents.wxs"), + "-d", "MediaTemplateCompressionLevel", + "-loc", Path.Combine(folder, "Package.en-us.wxl"), + "-bindpath", Path.Combine(folder, "data"), + "-intermediateFolder", intermediateFolder, + "-o", msiPath + }); + + result.AssertSuccess(); + Assert.True(File.Exists(cabPath)); + + var fileTable = Query.QueryDatabase(msiPath, new[] { "File" }); + var fileRows = fileTable.Select(r => new FileRow(r)).OrderBy(f => f.Sequence).ToList(); + + Assert.Equal(new[] { 1, 2 }, fileRows.Select(f => f.Sequence).ToArray()); + Assert.Equal(new[] { "test.txt", "Notepad.exe" }, fileRows.Select(f => f.Name).ToArray()); + + var files = Query.GetCabinetFiles(cabPath); + Assert.Equal(fileRows.Select(f => f.Id).ToArray(), files.Select(f => f.Name).ToArray()); + } + } + + private class FileRow + { + public FileRow(string row) + { + row = row.Substring("File:".Length); + + var split = row.Split('\t'); + this.Id = split[0]; + this.Name = split[2]; + this.Sequence = Convert.ToInt32(split[7]); + } + + public string Id { get; set; } + + public string Name { get; set; } + + public int Sequence { get; set; } + } + } +} diff --git a/src/test/WixToolsetTest.CoreIntegration/TestData/MultiFileCompressed/PackageComponents.wxs b/src/test/WixToolsetTest.CoreIntegration/TestData/MultiFileCompressed/PackageComponents.wxs index d65a07df..82797ebe 100644 --- a/src/test/WixToolsetTest.CoreIntegration/TestData/MultiFileCompressed/PackageComponents.wxs +++ b/src/test/WixToolsetTest.CoreIntegration/TestData/MultiFileCompressed/PackageComponents.wxs @@ -3,10 +3,10 @@ - + - + -- cgit v1.2.3-55-g6feb