From 89c476396d7ad6ab7c596300ec082dd9c38b9121 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Tue, 1 Mar 2022 20:10:52 -0800 Subject: Improve error reporting wrt ExePackage and MsuPackage attributes For example, DetectCondition is required when RepairArguments or UninstallArguments present and always recommended. Also, non-permanent ExePackages need UninstallArguments. The code was refactored to make it easier to reason over the different requirements for different package types. --- .../FrameworkDependentBundle.wxs | 2 +- .../EarliestCoreBundleSCD/SelfContainedBundle.wxs | 2 +- .../TrimmedSelfContainedBundle.wxs | 2 +- .../test/examples/FullFramework2Bundle/Bundle.wxs | 2 +- .../test/examples/FullFramework4Bundle/Bundle.wxs | 2 +- .../FrameworkDependentBundle.wxs | 2 +- .../LatestCoreBundleSCD/SelfContainedBundle.wxs | 2 +- .../TrimmedSelfContainedBundle.wxs | 2 +- .../WPFCoreBundleFDD/FrameworkDependentBundle.wxs | 2 +- .../TestData/DependencyTests/BundleE/BundleE.wxs | 2 +- src/wix/WixToolset.Core/Compiler_Bundle.cs | 106 ++++++++++++++++----- .../ExePackageFixture.cs | 6 ++ .../PayloadFixture.cs | 2 - .../TestData/BadInput/DuplicateCacheIds.wxs | 4 +- .../TestData/BadInput/DuplicatePayloadNames.wxs | 8 +- .../TestData/BadInput/UnscheduledPackage.wxs | 6 +- .../BadInput/UnscheduledRollbackBoundary.wxs | 4 +- .../Dependency/ExePackageProvidesBundle.wxs | 2 +- .../TestData/MsuPackage/Bundle.wxs | 2 +- .../Payload/DownloadUrlPlaceholdersBundle.wxs | 2 +- .../Payload/SharedBAAndPackagePayloadBundle.wxs | 2 +- .../SingleExeBundle/SingleExePackageGroup.wxs | 2 +- 22 files changed, 112 insertions(+), 54 deletions(-) (limited to 'src') diff --git a/src/ext/Bal/test/examples/EarliestCoreBundleFDD/FrameworkDependentBundle.wxs b/src/ext/Bal/test/examples/EarliestCoreBundleFDD/FrameworkDependentBundle.wxs index ce4d8db7..116d5223 100644 --- a/src/ext/Bal/test/examples/EarliestCoreBundleFDD/FrameworkDependentBundle.wxs +++ b/src/ext/Bal/test/examples/EarliestCoreBundleFDD/FrameworkDependentBundle.wxs @@ -9,7 +9,7 @@ - + diff --git a/src/ext/Bal/test/examples/EarliestCoreBundleSCD/SelfContainedBundle.wxs b/src/ext/Bal/test/examples/EarliestCoreBundleSCD/SelfContainedBundle.wxs index 3ff2a196..d888d3d9 100644 --- a/src/ext/Bal/test/examples/EarliestCoreBundleSCD/SelfContainedBundle.wxs +++ b/src/ext/Bal/test/examples/EarliestCoreBundleSCD/SelfContainedBundle.wxs @@ -5,7 +5,7 @@ - + diff --git a/src/ext/Bal/test/examples/EarliestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs b/src/ext/Bal/test/examples/EarliestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs index b0f7a831..5f1aa557 100644 --- a/src/ext/Bal/test/examples/EarliestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs +++ b/src/ext/Bal/test/examples/EarliestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs @@ -5,7 +5,7 @@ - + diff --git a/src/ext/Bal/test/examples/FullFramework2Bundle/Bundle.wxs b/src/ext/Bal/test/examples/FullFramework2Bundle/Bundle.wxs index f3a2b182..cdb15bb0 100644 --- a/src/ext/Bal/test/examples/FullFramework2Bundle/Bundle.wxs +++ b/src/ext/Bal/test/examples/FullFramework2Bundle/Bundle.wxs @@ -8,7 +8,7 @@ - + diff --git a/src/ext/Bal/test/examples/FullFramework4Bundle/Bundle.wxs b/src/ext/Bal/test/examples/FullFramework4Bundle/Bundle.wxs index 31f1eb9b..3602f969 100644 --- a/src/ext/Bal/test/examples/FullFramework4Bundle/Bundle.wxs +++ b/src/ext/Bal/test/examples/FullFramework4Bundle/Bundle.wxs @@ -8,7 +8,7 @@ - + diff --git a/src/ext/Bal/test/examples/LatestCoreBundleFDD/FrameworkDependentBundle.wxs b/src/ext/Bal/test/examples/LatestCoreBundleFDD/FrameworkDependentBundle.wxs index 78ac8220..b29363dd 100644 --- a/src/ext/Bal/test/examples/LatestCoreBundleFDD/FrameworkDependentBundle.wxs +++ b/src/ext/Bal/test/examples/LatestCoreBundleFDD/FrameworkDependentBundle.wxs @@ -9,7 +9,7 @@ - + diff --git a/src/ext/Bal/test/examples/LatestCoreBundleSCD/SelfContainedBundle.wxs b/src/ext/Bal/test/examples/LatestCoreBundleSCD/SelfContainedBundle.wxs index a708d0ad..56edc986 100644 --- a/src/ext/Bal/test/examples/LatestCoreBundleSCD/SelfContainedBundle.wxs +++ b/src/ext/Bal/test/examples/LatestCoreBundleSCD/SelfContainedBundle.wxs @@ -5,7 +5,7 @@ - + diff --git a/src/ext/Bal/test/examples/LatestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs b/src/ext/Bal/test/examples/LatestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs index f862d396..7ac9c34a 100644 --- a/src/ext/Bal/test/examples/LatestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs +++ b/src/ext/Bal/test/examples/LatestCoreBundleTrimmedSCD/TrimmedSelfContainedBundle.wxs @@ -5,7 +5,7 @@ - + diff --git a/src/ext/Bal/test/examples/WPFCoreBundleFDD/FrameworkDependentBundle.wxs b/src/ext/Bal/test/examples/WPFCoreBundleFDD/FrameworkDependentBundle.wxs index 8d9f5ca4..5f054ca4 100644 --- a/src/ext/Bal/test/examples/WPFCoreBundleFDD/FrameworkDependentBundle.wxs +++ b/src/ext/Bal/test/examples/WPFCoreBundleFDD/FrameworkDependentBundle.wxs @@ -9,7 +9,7 @@ - + diff --git a/src/test/burn/TestData/DependencyTests/BundleE/BundleE.wxs b/src/test/burn/TestData/DependencyTests/BundleE/BundleE.wxs index 03b3538b..a2372241 100644 --- a/src/test/burn/TestData/DependencyTests/BundleE/BundleE.wxs +++ b/src/test/burn/TestData/DependencyTests/BundleE/BundleE.wxs @@ -9,7 +9,7 @@ - diff --git a/src/wix/WixToolset.Core/Compiler_Bundle.cs b/src/wix/WixToolset.Core/Compiler_Bundle.cs index 3fde990e..dd263484 100644 --- a/src/wix/WixToolset.Core/Compiler_Bundle.cs +++ b/src/wix/WixToolset.Core/Compiler_Bundle.cs @@ -6,7 +6,6 @@ namespace WixToolset.Core using System.Collections.Generic; using System.Diagnostics; using System.Globalization; - using System.IO; using System.Linq; using System.Xml.Linq; using WixToolset.Data; @@ -2099,7 +2098,7 @@ namespace WixToolset.Core allowed = (packageType == WixBundlePackageType.Exe); break; case "UninstallArguments": - uninstallArguments = this.Core.GetAttributeValue(sourceLineNumbers, attrib); + uninstallArguments = this.Core.GetAttributeValue(sourceLineNumbers, attrib, EmptyRule.CanBeEmpty); allowed = (packageType == WixBundlePackageType.Exe); break; case "PerMachine": @@ -2187,49 +2186,104 @@ namespace WixToolset.Core rollbackPathVariable = String.Concat("WixBundleRollbackLog_", id.Id); } - if (!String.IsNullOrEmpty(protocol) && !protocol.Equals("burn", StringComparison.Ordinal) && !protocol.Equals("netfx4", StringComparison.Ordinal) && !protocol.Equals("none", StringComparison.Ordinal)) + if (packageType == WixBundlePackageType.Exe) { - this.Core.Write(ErrorMessages.IllegalAttributeValueWithLegalList(sourceLineNumbers, node.Name.LocalName, "Protocol", protocol, "none, burn, netfx4")); - } + // Set default scope for EXEs and MSPs if not already set. + if (perMachine == YesNoDefaultType.NotSet) + { + perMachine = YesNoDefaultType.Default; + } - if (!String.IsNullOrEmpty(protocol) && protocol.Equals("netfx4", StringComparison.Ordinal)) - { - foreach (var expectedArgument in expectedNetFx4Args) + if (permanent == YesNoType.No) { - if (null == installArguments || -1 == installArguments.IndexOf(expectedArgument, StringComparison.OrdinalIgnoreCase)) + if (uninstallArguments == null) { - this.Core.Write(WarningMessages.AttributeShouldContain(sourceLineNumbers, node.Name.LocalName, "InstallArguments", installArguments, expectedArgument, "Protocol", "netfx4")); + this.Core.Write(ErrorMessages.ExpectedAttribute(sourceLineNumbers, node.Name.LocalName, "UninstallArguments", "Permanent", "no")); } + } + else if (permanent == YesNoType.NotSet) + { + if (uninstallArguments == null) + { + this.Core.Write(ErrorMessages.ExpectedAttributeWithoutOtherAttribute(sourceLineNumbers, node.Name.LocalName, "UninstallArguments", "Permanent")); + } + } - if (!String.IsNullOrEmpty(repairArguments) && -1 == repairArguments.IndexOf(expectedArgument, StringComparison.OrdinalIgnoreCase)) + // Detect condition is recommended or required for Exe packages (depending on whether repair or uninstall arguments were provided). + if (String.IsNullOrEmpty(detectCondition)) + { + if (repairArguments != null) { - this.Core.Write(WarningMessages.AttributeShouldContain(sourceLineNumbers, node.Name.LocalName, "RepairArguments", repairArguments, expectedArgument, "Protocol", "netfx4")); + this.Core.Write(ErrorMessages.ExpectedAttributeWithValueWithOtherAttribute(sourceLineNumbers, node.Name.LocalName, "DetectCondition", "RepairArguments")); } + else if (uninstallArguments != null) + { + this.Core.Write(ErrorMessages.ExpectedAttributeWithValueWithOtherAttribute(sourceLineNumbers, node.Name.LocalName, "DetectCondition", "UninstallArguments")); + } + else + { + this.Core.Write(WarningMessages.DetectConditionRecommended(sourceLineNumbers, node.Name.LocalName)); + } + } + + // Validate the protocol if provided. + if (!String.IsNullOrEmpty(protocol)) + { + if (protocol.Equals("netfx4", StringComparison.Ordinal)) + { + foreach (var expectedArgument in expectedNetFx4Args) + { + if (null == installArguments || -1 == installArguments.IndexOf(expectedArgument, StringComparison.OrdinalIgnoreCase)) + { + this.Core.Write(WarningMessages.AttributeShouldContain(sourceLineNumbers, node.Name.LocalName, "InstallArguments", installArguments, expectedArgument, "Protocol", "netfx4")); + } - if (!String.IsNullOrEmpty(uninstallArguments) && -1 == uninstallArguments.IndexOf(expectedArgument, StringComparison.OrdinalIgnoreCase)) + if (!String.IsNullOrEmpty(repairArguments) && -1 == repairArguments.IndexOf(expectedArgument, StringComparison.OrdinalIgnoreCase)) + { + this.Core.Write(WarningMessages.AttributeShouldContain(sourceLineNumbers, node.Name.LocalName, "RepairArguments", repairArguments, expectedArgument, "Protocol", "netfx4")); + } + + if (!String.IsNullOrEmpty(uninstallArguments) && -1 == uninstallArguments.IndexOf(expectedArgument, StringComparison.OrdinalIgnoreCase)) + { + this.Core.Write(WarningMessages.AttributeShouldContain(sourceLineNumbers, node.Name.LocalName, "UninstallArguments", uninstallArguments, expectedArgument, "Protocol", "netfx4")); + } + } + } + else if (!protocol.Equals("burn", StringComparison.Ordinal) && !protocol.Equals("none", StringComparison.Ordinal)) { - this.Core.Write(WarningMessages.AttributeShouldContain(sourceLineNumbers, node.Name.LocalName, "UninstallArguments", uninstallArguments, expectedArgument, "Protocol", "netfx4")); + this.Core.Write(ErrorMessages.IllegalAttributeValueWithLegalList(sourceLineNumbers, node.Name.LocalName, "Protocol", protocol, "none, burn, netfx4")); } } } - - // Only set default scope for EXEs and MSPs if not already set. - if ((WixBundlePackageType.Exe == packageType || WixBundlePackageType.Msp == packageType) && YesNoDefaultType.NotSet == perMachine) + else if (packageType == WixBundlePackageType.Msp) { - perMachine = YesNoDefaultType.Default; + // Set default scope for EXEs and MSPs if not already set. + if (perMachine == YesNoDefaultType.NotSet) + { + perMachine = YesNoDefaultType.Default; + } } - - // Detect condition is recommended or required for Exe and Msu packages - // (depending on whether uninstall arguments were provided). - if ((packageType == WixBundlePackageType.Exe || packageType == WixBundlePackageType.Msu) && String.IsNullOrEmpty(detectCondition)) + else if (packageType == WixBundlePackageType.Msu) { - if (String.IsNullOrEmpty(uninstallArguments)) + if (permanent == YesNoType.No) { - this.Core.Write(WarningMessages.DetectConditionRecommended(sourceLineNumbers, node.Name.LocalName)); + if (String.IsNullOrEmpty(msuKB)) + { + this.Core.Write(ErrorMessages.ExpectedAttribute(sourceLineNumbers, node.Name.LocalName, "KB", "Permanent", "no")); + } } - else + else if (permanent == YesNoType.NotSet) + { + if (String.IsNullOrEmpty(msuKB)) + { + this.Core.Write(ErrorMessages.ExpectedAttributeWithoutOtherAttribute(sourceLineNumbers, node.Name.LocalName, "KB", "Permanent")); + } + } + + // Detect condition is recommended for Msu packages. + if (String.IsNullOrEmpty(detectCondition)) { - this.Core.Write(ErrorMessages.ExpectedAttributeWithValueWithOtherAttribute(sourceLineNumbers, node.Name.LocalName, "DetectCondition", "UninstallArguments")); + this.Core.Write(WarningMessages.DetectConditionRecommended(sourceLineNumbers, node.Name.LocalName)); } } diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/ExePackageFixture.cs b/src/wix/test/WixToolsetTest.CoreIntegration/ExePackageFixture.cs index e2306dcd..15885b94 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/ExePackageFixture.cs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/ExePackageFixture.cs @@ -3,6 +3,7 @@ namespace WixToolsetTest.CoreIntegration { using System.IO; + using System.Linq; using WixBuildTools.TestSupport; using WixToolset.Core.TestPackage; using Xunit; @@ -25,6 +26,11 @@ namespace WixToolsetTest.CoreIntegration "-o", Path.Combine(baseFolder, "test.wixlib") }); + WixAssert.CompareLineByLine(new[] + { + "The ExePackage element's UninstallArguments attribute was not found; it is required without attribute Permanent present.", + "The ExePackage/@DetectCondition attribute is recommended so the package is only installed when absent." + }, result.Messages.Select(m => m.ToString()).ToArray()); Assert.Equal(1153, result.ExitCode); } } diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/PayloadFixture.cs b/src/wix/test/WixToolsetTest.CoreIntegration/PayloadFixture.cs index bda642cb..5b663d93 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/PayloadFixture.cs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/PayloadFixture.cs @@ -2,13 +2,11 @@ namespace WixToolsetTest.CoreIntegration { - using System; using System.Collections.Generic; using System.IO; using System.Linq; using System.Xml; using WixBuildTools.TestSupport; - using WixToolset.Core; using WixToolset.Core.TestPackage; using WixToolset.Data; using WixToolset.Data.Symbols; diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs index 0c350042..f404d2ee 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicateCacheIds.wxs @@ -2,8 +2,8 @@ - - + + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicatePayloadNames.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicatePayloadNames.wxs index 4fe7e097..002aa18b 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicatePayloadNames.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/DuplicatePayloadNames.wxs @@ -2,12 +2,12 @@ - - - + + + - + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledPackage.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledPackage.wxs index fc53c4a2..5172fed0 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledPackage.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledPackage.wxs @@ -2,15 +2,15 @@ - - + + - + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledRollbackBoundary.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledRollbackBoundary.wxs index 6cf8528e..de67d001 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledRollbackBoundary.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/BadInput/UnscheduledRollbackBoundary.wxs @@ -2,8 +2,8 @@ - - + + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Dependency/ExePackageProvidesBundle.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Dependency/ExePackageProvidesBundle.wxs index 4d188d3a..2f7d41c7 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Dependency/ExePackageProvidesBundle.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Dependency/ExePackageProvidesBundle.wxs @@ -2,7 +2,7 @@ - + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/MsuPackage/Bundle.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/MsuPackage/Bundle.wxs index dbca3393..48222546 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/MsuPackage/Bundle.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/MsuPackage/Bundle.wxs @@ -5,7 +5,7 @@ - + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/DownloadUrlPlaceholdersBundle.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/DownloadUrlPlaceholdersBundle.wxs index f8f38ea6..f077c418 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/DownloadUrlPlaceholdersBundle.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/DownloadUrlPlaceholdersBundle.wxs @@ -14,7 +14,7 @@ - + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/SharedBAAndPackagePayloadBundle.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/SharedBAAndPackagePayloadBundle.wxs index 5263cbd4..5bfa2972 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/SharedBAAndPackagePayloadBundle.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/Payload/SharedBAAndPackagePayloadBundle.wxs @@ -2,7 +2,7 @@ - + diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/SingleExeBundle/SingleExePackageGroup.wxs b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/SingleExeBundle/SingleExePackageGroup.wxs index cad1f049..ce0db542 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/TestData/SingleExeBundle/SingleExePackageGroup.wxs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/TestData/SingleExeBundle/SingleExePackageGroup.wxs @@ -2,7 +2,7 @@ - + -- cgit v1.2.3-55-g6feb