Use wiutil to start/end msi transactions #29

nirbar · 2021-01-12T06:39:37Z

Add logging for transactions
Let BA vote whether or not to use the transaction
Warn if transactions aren't suported on the target machine
See wixtoolset/issues#5386

rseanhall · 2021-01-12T07:04:31Z

There's way too much going on here for one pull request. Let's start with the first one - using wiutil to start/end msi transactions. Please get rid of everything else. It's up to you whether to keep this one open and force push, or close this and open a new one. You must keep calling the methods in msiengine, make the changes there.

rseanhall

Having to keep track of the handles made things really messy. I really hope we can avoid that.

rseanhall · 2021-01-12T15:05:39Z

burn.sln

@@ -6,6 +6,9 @@ MinimumVisualStudioVersion = 15.0.26124.0
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "engine", "src\engine\engine.vcxproj", "{8119537D-E1D9-6591-D51A-49768A2F9C37}"
 EndProject
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "stub", "src\stub\stub.vcxproj", "{C38373AA-882F-4F55-B03F-2AAB4BFBE3F1}"
+	ProjectSection(ProjectDependencies) = postProject
+		{8119537D-E1D9-6591-D51A-49768A2F9C37} = {8119537D-E1D9-6591-D51A-49768A2F9C37}
+	EndProjectSection


This should be reverted.

I fixed the root issue (incomplete ProjectReference) in #37.

rseanhall · 2021-01-12T15:05:53Z

nuget.config

@@ -1,7 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <configuration>
  <packageSources>
-    <clear />


This should be reverted.

rseanhall · 2021-01-12T15:08:02Z

src/engine/apply.cpp

+                // If inside a MSI transaction, roll it back.
+                if (pCheckpoint->pActiveRollbackBoundary && pCheckpoint->pActiveRollbackBoundary->fActiveTransaction)
+                {
+                    hrRollback = ExecuteMsiRollbackTransaction(pEngineState, pCheckpoint->pActiveRollbackBoundary, &context);
+                    IgnoreRollbackError(hrRollback, "Failed rolling back transaction");
+                }
+                else
+                {
+                    hrRollback = DoRollbackActions(pEngineState, &context, pCheckpoint->dwId, pfKeepRegistration, pRestart);
+                    IgnoreRollbackError(hrRollback, "Failed rollback actions");
+                }


This is wrong. DoRollbackActions needs to be executed even if there was an active MSI transaction. I have already modified the plan so that it doesn't have rollback actions for the MSI/MSP packages.

rseanhall · 2021-01-12T15:11:14Z

src/engine/apply.cpp

@@ -830,6 +835,7 @@ extern "C" HRESULT ApplyExecute(
            }

            // Move forward to next rollback boundary.
+            hr = S_OK;


I don't want to take this change in this pull request, it seems to belong to a completely different bug.

rseanhall · 2021-01-12T15:16:31Z

src/engine/engine.mc

+SymbolicName=MSG_UX_DECLINED_MSI_TRANSACTION
+Language=English
+UX voted against MSI transaction on rollback boundary '%1!ls!'
+.


These messages should be removed.

rseanhall · 2021-01-12T15:32:41Z

src/engine/variable.cpp

@@ -157,10 +157,12 @@ static HRESULT InitializeVariableNumeric(
    __in DWORD_PTR dwpData,
    __inout BURN_VARIANT* pValue
    );
+#if !defined(_WIN64)


The changes in this file should be reverted.

rseanhall · 2021-01-12T15:34:09Z

src/test/BurnUnitTest/PlanTest.cpp

@@ -641,6 +641,9 @@ namespace Bootstrapper
            ::InitializeCriticalSection(&pEngineState->csActive);
            ::InitializeCriticalSection(&pEngineState->userExperience.csEngineActive);

+            hr = WiuInitialize();


This should no longer be necessary when the call to WiuIsMsiTransactionSupported is removed.

src/engine/msiengine.cpp

rseanhall · 2021-01-12T15:43:01Z

src/engine/msiengine.cpp

-    uResult = ::MsiBeginTransaction(wzName, 0, &hTransactionHandle, &hChangeOfOwnerEvent);
-
-    if (ERROR_ROLLBACK_DISABLED == uResult)
+    if (E_ROLLBACK_DISABLED == hr)


Good catch. I don't see us defining a new HRESULT value from a Win32 error anywhere else in Burn. Since it's used only once, I would rather do HRESULT_FROM_WIN32(ERROR_ROLLBACK_DISABLED) inline.

src/engine/elevation.h

rseanhall · 2021-01-13T15:58:32Z

I've been thinking about this, and you might as well include the real logging logic in this pull request as well.

There needs to be dwLoggingAttributes and sczLogPath in the rollback boundary struct which needs to be calculated during plan, like msiengine. The handles could also be stored there. The MsiEngine*Transaction methods would need the struct as a parameter instead of the id.

burn/src/engine/msiengine.cpp

Lines 980 to 981 in 5bab3f6

    
           LoggingSetPackageVariable(pPackage, NULL, TRUE, pLog, pVariables, &pAction->msiPackage.sczLogPath); // ignore errors. 
        
           pAction->msiPackage.dwLoggingAttributes = pLog->dwAttributes;

You would have to pass dwLoggingAttributes and sczLogPath along with the id to the elevated engine when creating the transaction, where it would use the id to look up the rollback boundary struct like it does when looking up an MSI package. You would store the log parameters there. The other transaction methods would need to keep requiring the id so they can look up the rollback boundary.

burn/src/engine/elevation.cpp

Lines 2322 to 2323 in 5bab3f6

    
           hr = PackageFindById(pPackages, sczPackage, &executeAction.msiPackage.pPackage); 
        
           ExitOnFailure(hr, "Failed to find package: %ls", sczPackage);

dwLoggingAttributes is used for adding INSTALLLOGMODE_EXTRADEBUG.

burn/src/engine/msiengine.cpp

Lines 1249 to 1255 in 5bab3f6

    
           // Default to "verbose" logging and set extra debug mode only if explicitly required. 
        
           DWORD dwLogMode = WIU_LOG_DEFAULT | INSTALLLOGMODE_VERBOSE; 
        
           if (pExecuteAction->msiPackage.dwLoggingAttributes & BURN_LOGGING_ATTRIBUTE_EXTRADEBUG) 
        
           { 
        
               dwLogMode |= INSTALLLOGMODE_EXTRADEBUG; 
        
           }

rseanhall

Have you been able to make any progress? I would like to start getting these MSI transaction changes in.

rseanhall · 2021-02-08T01:55:02Z

burn.sln

@@ -6,6 +6,9 @@ MinimumVisualStudioVersion = 15.0.26124.0
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "engine", "src\engine\engine.vcxproj", "{8119537D-E1D9-6591-D51A-49768A2F9C37}"
 EndProject
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "stub", "src\stub\stub.vcxproj", "{C38373AA-882F-4F55-B03F-2AAB4BFBE3F1}"
+	ProjectSection(ProjectDependencies) = postProject
+		{8119537D-E1D9-6591-D51A-49768A2F9C37} = {8119537D-E1D9-6591-D51A-49768A2F9C37}
+	EndProjectSection


I fixed the root issue (incomplete ProjectReference) in #37.

rseanhall · 2021-02-08T01:57:46Z

src/engine/apply.cpp

    )
 {
    HRESULT hr = S_OK;
-    BOOL fBeginCalled = FALSE;
+    BOOL fCommitCalled = FALSE;


This was shorthand for "fOnCommitMsiTransactionBeginCalled". There are many places where we're using fBeginCalled to track whether the BA *Begin method was called. If you find this confusing, then I would prefer fCommitBeginCalled.

rseanhall · 2021-02-08T01:58:30Z

src/engine/plan.cpp

@@ -1825,6 +1825,13 @@ extern "C" HRESULT PlanRollbackBoundaryBegin(
    AssertSz(!pPlan->pActiveRollbackBoundary, "PlanRollbackBoundaryBegin called without completing previous RollbackBoundary");
    pPlan->pActiveRollbackBoundary = pRollbackBoundary;

+    // Best effort to support MSI transactions
+    if (pRollbackBoundary->fTransaction && !WiuIsMsiTransactionSupported())


This was done in #37.

nirbar · 2021-02-08T06:50:06Z

I have some clients nearing product release dates, so it's quite busy currently.
I'll get back to it in a few weeks

rseanhall · 2021-03-16T16:45:45Z

@nirbar Ping.

Add logging for transactions Let BA vote whether or not to use the transaction Warn if transactions aren't suported on the target machine

rseanhall · 2021-03-17T19:48:56Z

Thanks. I took the changes for using wiutil and closing the handles immediately and submitted it here: #47. I look forward to the next pull request with the logging enhancements or the BA involvement.

nirbar force-pushed the nirbar-5386 branch from 587deb4 to 735e277 Compare January 12, 2021 12:17

rseanhall reviewed Jan 12, 2021

View reviewed changes

rseanhall reviewed Feb 8, 2021

View reviewed changes

nirbar added 3 commits March 17, 2021 11:24

Use wiutil to start/end msi transactions

1b6625c

Add logging for transactions Let BA vote whether or not to use the transaction Warn if transactions aren't suported on the target machine

Revert some changes- log path variable, BA plan MSI transaction

1f81904

Release MSI transaction handles immediately

2e15554

nirbar force-pushed the nirbar-5386 branch from 735e277 to 2e15554 Compare March 17, 2021 09:32

Fix CR comments

6e849f3

rseanhall mentioned this pull request Mar 17, 2021

Use wiutil to start/end msi transactions #47

Merged

rseanhall closed this Mar 17, 2021

This was referenced Apr 26, 2021

Nirbar 5386 #53

Closed

BA Plan & log transaction #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use wiutil to start/end msi transactions #29

Use wiutil to start/end msi transactions #29

nirbar commented Jan 12, 2021

rseanhall commented Jan 12, 2021

rseanhall left a comment

rseanhall Jan 12, 2021

rseanhall Feb 8, 2021

rseanhall Jan 12, 2021

rseanhall Jan 12, 2021

rseanhall Jan 12, 2021

rseanhall Jan 12, 2021

rseanhall Jan 12, 2021

rseanhall Jan 12, 2021

rseanhall Jan 12, 2021

rseanhall commented Jan 13, 2021

rseanhall left a comment

rseanhall Feb 8, 2021

rseanhall Feb 8, 2021

rseanhall Feb 8, 2021

nirbar commented Feb 8, 2021

rseanhall commented Mar 16, 2021

rseanhall commented Mar 17, 2021

Use wiutil to start/end msi transactions #29

Use wiutil to start/end msi transactions #29

Conversation

nirbar commented Jan 12, 2021

rseanhall commented Jan 12, 2021

rseanhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rseanhall commented Jan 13, 2021

rseanhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirbar commented Feb 8, 2021

rseanhall commented Mar 16, 2021

rseanhall commented Mar 17, 2021