On Tue, Mar 4, 2008 at 11:51 AM, Ove Kaaven ovek@transgaming.com wrote:
The email address in the patch was wrong, sending it again.
From b88a43dbc2234c8b35ee4fa7bb4f275475b8d821 Mon Sep 17 00:00:00 2001
From: Ove Kaaven ovek@transgaming.com Date: Tue, 4 Mar 2008 18:16:27 +0100 Subject: [PATCH] Fix bug in handling of multivolume CAB files. After extracting a single file from a multivolume CAB file, sometimes information about the next CAB file to extract from would not be loaded, because the is_continuous state was previously only properly reset if a file happened to be partially extracted from the last volume of a multivolume series.
A lot of time has been spent fixing cab/media related bugs in installers, as the behavior is very fickle (and easily breakable), so can you please add a test case first? See the multicab tests that are already in install.c. They will fail in wine because our cabinet.dll cannot create compressed cabinets, but assuming you test in Windows or use native cabinet.dll, it'll still verify that the patch is correct.
James Hawkins skrev:
A lot of time has been spent fixing cab/media related bugs in installers, as the behavior is very fickle (and easily breakable), so can you please add a test case first? See the multicab tests that are already in install.c. They will fail in wine because our cabinet.dll cannot create compressed cabinets, but assuming you test in Windows or use native cabinet.dll, it'll still verify that the patch is correct.
Using native cabinet.dll from XP causes numerous failures in those tests (notably in caborder, if I remember right). I probably won't really have time to track that down right now.
But it seems that I am able to use builtin cabinet if I change create_cc_test_files to use a maxsize of 40000 or whatever instead of 200, so that it creates 2 cabfiles instead of 615. (That routine creates something close to the situation I originally described, too.) And if I try it that way, my patch seems to fix the todo tests at lines 1733 and 1734 (in test_uiLevelFlags), for some reason. Is that good enough?
On Tue, Mar 4, 2008 at 7:01 PM, Ove Kaaven ovek@transgaming.com wrote:
James Hawkins skrev:
A lot of time has been spent fixing cab/media related bugs in installers, as the behavior is very fickle (and easily breakable), so can you please add a test case first? See the multicab tests that are already in install.c. They will fail in wine because our cabinet.dll cannot create compressed cabinets, but assuming you test in Windows or use native cabinet.dll, it'll still verify that the patch is correct.
Using native cabinet.dll from XP causes numerous failures in those tests (notably in caborder, if I remember right). I probably won't really have time to track that down right now.
But it seems that I am able to use builtin cabinet if I change create_cc_test_files to use a maxsize of 40000 or whatever instead of 200, so that it creates 2 cabfiles instead of 615. (That routine creates something close to the situation I originally described, too.) And if I try it that way, my patch seems to fix the todo tests at lines 1733 and 1734 (in test_uiLevelFlags), for some reason. Is that good enough?
The tests that now pass with native cabinet dll are test_continuouscab (which is similar to what you're trying to test). The point of maxsize is so that it creates continuous cabs...there's no other way to do it, and builtin doesn't create continuous cabs at all. The answer to your question is no, because there is no test currently that runs down the code path you are fixing.
James Hawkins skrev:
The tests that now pass with native cabinet dll are test_continuouscab (which is similar to what you're trying to test). The point of maxsize is so that it creates continuous cabs...there's no other way to do it, and builtin doesn't create continuous cabs at all.
No? Then I really wonder what those hundreds of .cab files it creates from that 50k testfile is, if not continuous cabs. What else are they?
The answer to your question is no, because there is no test currently that runs down the code path you are fixing.
Looks like it takes an order of magnitude more time writing test cases for this stuff than actually fixing that darn bug (fixing the bug took 2 hours, figuring out how to put a working test case together (trying to take differences between native and builtin cabinet into account) has taken me all day, probably longer).
Here's what I managed to put together that seems to trigger the problem condition... the cab extraction sequence was somehow different in my real-world case, so I had to fudge this testcase a little by packing a third file into the continuous cab. So what do you think about this test, then?
diff --git a/dlls/msi/tests/install.c b/dlls/msi/tests/install.c index 1f64010..e19c4c1 100644 --- a/dlls/msi/tests/install.c +++ b/dlls/msi/tests/install.c @@ -175,6 +175,13 @@ static const CHAR cc_component_dat[] = "Component\tComponentId\tDirectory_\tAttr "augustus\t\tMSITESTDIR\t0\t1\taugustus\n" "caesar\t\tMSITESTDIR\t0\t1\tcaesar\n";
+static const CHAR cc2_component_dat[] = "Component\tComponentId\tDirectory_\tAttributes\tCondition\tKeyPath\n" + "s72\tS38\ts72\ti2\tS255\tS72\n" + "Component\tComponent\n" + "maximus\t\tMSITESTDIR\t0\t1\tmaximus\n" + "augustus\t\tMSITESTDIR\t0\t0\taugustus\n" + "caesar\t\tMSITESTDIR\t0\t1\tcaesar\n"; + static const CHAR cc_feature_dat[] = "Feature\tFeature_Parent\tTitle\tDescription\tDisplay\tLevel\tDirectory_\tAttributes\n" "s38\tS38\tL64\tL255\tI2\ti2\tS72\ti2\n" "Feature\tFeature\n" @@ -194,6 +201,14 @@ static const CHAR cc_file_dat[] = "File\tComponent_\tFileName\tFileSize\tVersion "augustus\taugustus\taugustus\t50000\t\t\t16384\t2\n" "caesar\tcaesar\tcaesar\t500\t\t\t16384\t12";
+static const CHAR cc2_file_dat[] = "File\tComponent_\tFileName\tFileSize\tVersion\tLanguage\tAttributes\tSequence\n" + "s72\ts72\tl255\ti4\tS72\tS20\tI2\ti2\n" + "File\tFile\n" + "maximus\tmaximus\tmaximus\t500\t\t\t16384\t1\n" + "augustus\taugustus\taugustus\t50000\t\t\t16384\t2\n" + "tiberius\tmaximus\ttiberius\t500\t\t\t16384\t3\n" + "caesar\tcaesar\tcaesar\t500\t\t\t16384\t12"; + static const CHAR cc_media_dat[] = "DiskId\tLastSequence\tDiskPrompt\tCabinet\tVolumeLabel\tSource\n" "i2\ti4\tL64\tS255\tS32\tS72\n" "Media\tDiskId\n" @@ -624,6 +639,18 @@ static const msi_table cc_tables[] = ADD_TABLE(property), };
+static const msi_table cc2_tables[] = +{ + ADD_TABLE(cc2_component), + ADD_TABLE(directory), + ADD_TABLE(cc_feature), + ADD_TABLE(cc_feature_comp), + ADD_TABLE(cc2_file), + ADD_TABLE(install_exec_seq), + ADD_TABLE(cc_media), + ADD_TABLE(property), +}; + static const msi_table co_tables[] = { ADD_TABLE(cc_component), @@ -1538,6 +1565,49 @@ static void create_cc_test_files(void) DeleteFile("caesar"); }
+static void create_cc2_test_files(void) +{ + CCAB cabParams; + HFCI hfci; + ERF erf; + static CHAR cab_context[] = "test%d.cab"; + BOOL res; + + create_file("maximus", 500); + create_file("augustus", 50000); + create_file("tiberius", 500); + create_file("caesar", 500); + + set_cab_parameters(&cabParams, "test1.cab", 40000); + + hfci = FCICreate(&erf, file_placed, mem_alloc, mem_free, fci_open, + fci_read, fci_write, fci_close, fci_seek, fci_delete, + get_temp_file, &cabParams, cab_context); + ok(hfci != NULL, "Failed to create an FCI context\n"); + + res = add_file(hfci, "maximus", tcompTYPE_NONE); + ok(res, "Failed to add file maximus\n"); + + res = add_file(hfci, "augustus", tcompTYPE_NONE); + ok(res, "Failed to add file augustus\n"); + + res = add_file(hfci, "tiberius", tcompTYPE_NONE); + ok(res, "Failed to add file tiberius\n"); + + res = FCIFlushCabinet(hfci, FALSE, get_next_cabinet, progress); + ok(res, "Failed to flush the cabinet\n"); + + res = FCIDestroy(hfci); + ok(res, "Failed to destroy the cabinet\n"); + + create_cab_file("test3.cab", MEDIA_SIZE, "caesar\0"); + + DeleteFile("maximus"); + DeleteFile("augustus"); + DeleteFile("tiberius"); + DeleteFile("caesar"); +} + static void delete_cab_files(void) { SHFILEOPSTRUCT shfl; @@ -1577,6 +1647,22 @@ static void test_continuouscabs(void)
delete_cab_files(); DeleteFile(msifile); + + create_cc2_test_files(); + create_database(msifile, cc2_tables, sizeof(cc2_tables) / sizeof(msi_table)); + + MsiSetInternalUI(INSTALLUILEVEL_NONE, NULL); + + r = MsiInstallProductA(msifile, NULL); + ok(r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", r); + ok(delete_pf("msitest\maximus", TRUE), "File not installed\n"); + ok(!delete_pf("msitest\augustus", TRUE), "File installed\n"); + ok(delete_pf("msitest\tiberius", TRUE), "File not installed\n"); + ok(delete_pf("msitest\caesar", TRUE), "File not installed\n"); + ok(delete_pf("msitest", FALSE), "File not installed\n"); + + delete_cab_files(); + DeleteFile(msifile); }
static void test_caborder(void)
On Wed, Mar 5, 2008 at 9:15 PM, Ove Kaaven ovek@transgaming.com wrote:
James Hawkins skrev:
The tests that now pass with native cabinet dll are test_continuouscab (which is similar to what you're trying to test). The point of maxsize is so that it creates continuous cabs...there's no other way to do it, and builtin doesn't create continuous cabs at all.
No? Then I really wonder what those hundreds of .cab files it creates from that 50k testfile is, if not continuous cabs. What else are they?
Sorry, I meant that it doesn't create the compressed continuous cabs that the tests expect.
The answer to your question is no, because there is no test currently that runs down the code path you are fixing.
Looks like it takes an order of magnitude more time writing test cases for this stuff than actually fixing that darn bug (fixing the bug took 2 hours, figuring out how to put a working test case together (trying to take differences between native and builtin cabinet into account) has taken me all day, probably longer).
You have to keep in consideration all the time that has been saved fixing bugs later down the road because the test cases have kept regressions to a minimum (and just flat out wrong code being added).
Here's what I managed to put together that seems to trigger the problem condition... the cab extraction sequence was somehow different in my real-world case, so I had to fudge this testcase a little by packing a third file into the continuous cab. So what do you think about this test, then?
Looks good to me, as long as the test fails without your fix and succeeds with your fix (with the cabinet override of course). When submitting the tests to wine-patches, make sure you add todo_wine to the failing tests (because of cabinet).
James Hawkins skrev:
On Wed, Mar 5, 2008 at 9:15 PM, Ove Kaaven ovek@transgaming.com wrote:
James Hawkins skrev:
The tests that now pass with native cabinet dll are test_continuouscab (which is similar to what you're trying to test). The point of maxsize is so that it creates continuous cabs...there's no other way to do it, and builtin doesn't create continuous cabs at all.
No? Then I really wonder what those hundreds of .cab files it creates from that 50k testfile is, if not continuous cabs. What else are they?
Sorry, I meant that it doesn't create the compressed continuous cabs that the tests expect.
Well, that's because create_cc_test_files expect to get 2 cab files, not 600, and so the "caesar" part proceeds to overwrite the 3rd in the series, rendering the whole series unreadable. By adjusting the max size so that only 2 (uncompressed) cab files are created, most of the todo tests appear to run fine. Just changing the file name might also work. (Actually I don't see any reason your tests would really need to depend on compression anyway, it's supposed to be a test of MSI, not cabinet.)
Here's what I managed to put together that seems to trigger the problem condition... the cab extraction sequence was somehow different in my real-world case, so I had to fudge this testcase a little by packing a third file into the continuous cab. So what do you think about this test, then?
Looks good to me, as long as the test fails without your fix and succeeds with your fix (with the cabinet override of course).
Actually, I think it works with both builtin and native cabinet, because my create_cc2_test_files creates uncompressed continuous cabinets, which both builtin and native seem to handle. (I was almost forced to use uncompressed anyway, because create_file creates sparse files, which compresses into almost nothing, which made it difficult to predict whether the contents would be distributed the way I needed...)
-
James Hawkins -
Ove Kaaven