Bug #4792 #268

Merged
merged 3 commits into from Jun 16, 2015

Projects

None yet

4 participants

@stunney
Contributor
stunney commented Jun 10, 2015

Adding validation of WixPdb to paired MSI to ensure that the two
actually belong together.

@stunney stunney Bug #4792
Adding validation of WixPdb to paired MSI to ensure that the two
actually belong together.
467d90d
@robmen
Member
robmen commented Jun 11, 2015

Two very big things:

  1. Only check the PackageCode. That will be an exact match.
  2. Don't send messages by throwing exceptions. Write a message like all the other code.
@stunney
Contributor
stunney commented Jun 11, 2015
  1. Agreed
  2. Crap, my bad.
@stunney
Contributor
stunney commented Jun 11, 2015

Would you recommend that melt still "abort" upon discovering the mismatch or simply serve up a "WARNING" on the console?

@robmen
Member
robmen commented Jun 12, 2015

For WiX v3.x I think it has to be a warning since maybe someone is doing this and it is working for them (magically). It should become an error in v4.x.

@stunney
Contributor
stunney commented Jun 12, 2015

Already added the warning string to the resource file.

@stunney
Contributor
stunney commented Jun 12, 2015

New PR will be up tonight.

@robmen
Member
robmen commented Jun 12, 2015

You don't need a new PR, you can just update your branch and push back to it. That will update this pull request.

@stunney stunney Bug #4792 - feedback updates
Based on feedback from Rob here:
#268
Using proper resource string for messages + Console instead of
exceptions.
Only checking ProductCode since it is the one true definition of a
unique MSI creation
c0fcbbb
@stunney
Contributor
stunney commented Jun 12, 2015

updates committed

@heaths heaths and 1 other commented on an outdated diff Jun 12, 2015
src/tools/melt/melt.cs
@@ -308,6 +308,30 @@ private static void MeltBinaryTable(Pdb inputPdb, InstallPackage package, string
}
}
+ private static bool ValidateMSIMatchesPdb(InstallPackage package, Pdb inputPdb)
+ {
+ const string KEY_PRODUCT_CODE = "ProductCode";
@heaths
heaths Jun 12, 2015 Contributor

By declaring const you're actually causing the string to be allocated multiple times. Either declare as static readonly at the class scope or declare here without const so the same string instance is used.

@stunney
stunney Jun 13, 2015 Contributor

Can't use static readonly at the class level and maintain the usage of the switch statement. That said, it is a single check so I can easily switch that to an if statement.

@stunney stunney Bug #4792 - continued feedback updates
More feedback from Heath.  Moved KEY_PRODUCT_CODE out as static readonly
in class definition.  Added a break statement to get out of the for loop
as soon as we have looks at the "ProductCode" row.  Added missing method
comments.
aa540d1
@stunney
Contributor
stunney commented Jun 13, 2015

Updates based on @heaths feedback committed.

@barnson barnson merged commit aa540d1 into wixtoolset:develop Jun 16, 2015
@rseanhall rseanhall added a commit to rseanhall/wix4 that referenced this pull request Aug 24, 2015
@stunney @rseanhall stunney + rseanhall Bug #4792 - feedback updates
Based on feedback from Rob here:
wixtoolset/wix3#268
Using proper resource string for messages + Console instead of
exceptions.
Only checking ProductCode since it is the one true definition of a
unique MSI creation

Conflicts:
	src/tools/melt/MeltStrings.resx
	src/tools/melt/melt.cs
d4e0615
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment