Add burn support for only caching packages #213

Merged
merged 8 commits into from Jun 7, 2015

Projects

None yet

5 participants

@heaths
Contributor
heaths commented Feb 28, 2015

Rebased and squashed on top of the current develop HEAD. This does not include the async install as there was some debate about it and we do not think we'll need it going forward anyway.

@barnson
Member
barnson commented Mar 1, 2015

Let's talk about this at Tuesday's meeting. We'd talked about making this something the BA could tell the engine to do rather than making it an all-engine job.

@heaths
Contributor
heaths commented Mar 1, 2015

My $0.02 in case I can't make triage: take this into wix3 as-is. Any changes to the BA will likely require an interface change which I know @robmen is rethinking for wix4.

@heaths
Contributor
heaths commented Mar 4, 2015

If this looks good, I'll make the same changes for the wix4 branch. @rseanhall , please have a look as well since I know you have a related PR.

@robmen robmen and 2 others commented on an outdated diff Mar 10, 2015
src/burn/engine/apply.cpp
@@ -452,6 +454,9 @@ extern "C" HRESULT ApplyCache(
hr = UserExperienceInterpretExecuteResult(pUX, FALSE, MB_OKCANCEL, nResult);
ExitOnRootFailure(hr, "UX aborted cache.");
+ // When in cache mode, we want to download in the background (if the download mechanism supports it).
+ BOOL fPreferBackgroundDownload = (BOOTSTRAPPER_ACTION_CACHE == pPlan->action);
@robmen
robmen Mar 10, 2015 Member

This seems pretty magical. Seems like BITS needs more callbacks (or something) to handle it's priority. Not great answer here but this may be the best can do in v3.x. @barnson, thoughts?

@barnson
barnson Mar 10, 2015 Member

I worry about using the "idle" levels for something with UI. But not worth an interface change, for sure.

@heaths
heaths May 9, 2015 Contributor

Background downloads removed. Originally it was to support async execution anyway, so probably didn't fit well with this change anyway.

@robmen robmen and 1 other commented on an outdated diff Mar 10, 2015
src/burn/engine/bitsengine.cpp
@@ -439,11 +442,19 @@ static HRESULT CreateJob(
hr = pJob->SetNotifyFlags(BG_NOTIFY_JOB_TRANSFERRED | BG_NOTIFY_JOB_ERROR | BG_NOTIFY_JOB_MODIFICATION);
ExitOnFailure(hr, "Failed to set notification flags for BITS job.");
- hr = pJob->SetNoProgressTimeout(BITSENGINE_NO_PROGRESS_TIMEOUT); // use 2 minutes since default is 14 days.
- ExitOnFailure(hr, "Failed to set progress timeout.");
+ if (fBackgroundDownload)
+ {
+ hr = pJob->SetPriority(BG_JOB_PRIORITY_LOW);
@robmen
robmen Mar 10, 2015 Member

Does this mean the Bundle could be running for basically ever (i.e. days)? Big bundle, only downloading in background... hangs around for long time, right?

@heaths
heaths Mar 12, 2015 Contributor

Yes, and the bundles we're doing this on are big - Windows Phone and Android emulators. This went with the async feature so with that it didn't matter (to answer @rseanhall's question about why the PR from last spring was closed, since OSS didn't want async execution). Without async, backgrounding a BITS job may not make sense.

@robmen robmen and 1 other commented on an outdated diff Mar 10, 2015
src/burn/engine/core.cpp
@@ -1508,8 +1508,8 @@ static void LogPackages(
LogId(REPORT_STANDARD, MSG_PLANNED_PACKAGE, pPackage->sczId, LoggingPackageStateToString(pPackage->currentState), LoggingRequestStateToString(pPackage->defaultRequested), LoggingRequestStateToString(pPackage->requested), LoggingActionStateToString(pPackage->execute), LoggingActionStateToString(pPackage->rollback), LoggingBoolToString(pPackage->fAcquire), LoggingBoolToString(pPackage->fUncache), LoggingDependencyActionToString(pPackage->dependencyExecute));
}
- // Display related bundles last if installing, modifying, or repairing.
- if (BOOTSTRAPPER_ACTION_UNINSTALL < action && 0 < pRelatedBundles->cRelatedBundles)
+ // Display related bundles last if caching, installing, modifying, or repairing.
+ if ((BOOTSTRAPPER_ACTION_UNINSTALL < action || BOOTSTRAPPER_ACTION_CACHE == action) && 0 < pRelatedBundles->cRelatedBundles)
@robmen
robmen Mar 10, 2015 Member

This change on line 1512 should be unnecessary when BOOTSTRAPPER_ACTION_CACHE is put after UNINSTALL, right?

@heaths
heaths Mar 12, 2015 Contributor

Correct, but with @rseanhall's PR committed does that even matter now?

@robmen
robmen Mar 12, 2015 Member

Yes, but @rseanhall didn't add the BOOTSTRAPPE_ACTION_CACHE code. That's still necessary to plan a CACHE operation, right?

@heaths
heaths Mar 12, 2015 Contributor

But from his comment on his PR, he added support by allowing a BA to mark all packages for Cache instead of, for example, Install. Or did I misunderstand what his PR was doing?

@robmen robmen commented on an outdated diff Mar 10, 2015
src/burn/engine/detect.cpp
@@ -172,7 +172,7 @@ extern "C" HRESULT DetectReportRelatedBundles(
switch (pRelatedBundle->relationType)
{
case BOOTSTRAPPER_RELATION_UPGRADE:
- if (BOOTSTRAPPER_RELATION_UPGRADE != relationType && BOOTSTRAPPER_ACTION_UNINSTALL < action)
+ if (BOOTSTRAPPER_RELATION_UPGRADE != relationType && (BOOTSTRAPPER_ACTION_UNINSTALL < action || BOOTSTRAPPER_ACTION_CACHE == action))
@robmen
robmen Mar 10, 2015 Member

Another unnecessary check when moving _CACHE after _UNINSTALL, correct?

@robmen robmen commented on an outdated diff Mar 10, 2015
src/burn/engine/plan.cpp
@@ -1237,7 +1238,7 @@ extern "C" HRESULT PlanRelatedBundlesBegin(
switch (pRelatedBundle->relationType)
{
case BOOTSTRAPPER_RELATION_UPGRADE:
- if (BOOTSTRAPPER_RELATION_UPGRADE != relationType && BOOTSTRAPPER_ACTION_UNINSTALL < pPlan->action)
+ if (BOOTSTRAPPER_RELATION_UPGRADE != relationType && (BOOTSTRAPPER_ACTION_UNINSTALL < pPlan->action || BOOTSTRAPPER_ACTION_CACHE == pPlan->action))
@robmen
robmen Mar 10, 2015 Member

One more.

@robmen robmen and 1 other commented on an outdated diff Mar 10, 2015
src/burn/engine/plan.cpp
@@ -1922,7 +1923,8 @@ static HRESULT GetActionDefaultRequestState(
{
case BOOTSTRAPPER_ACTION_INSTALL: __fallthrough;
case BOOTSTRAPPER_ACTION_UPDATE_REPLACE: __fallthrough;
- case BOOTSTRAPPER_ACTION_UPDATE_REPLACE_EMBEDDED:
+ case BOOTSTRAPPER_ACTION_UPDATE_REPLACE_EMBEDDED: __fallthrough;
+ case BOOTSTRAPPER_ACTION_CACHE:
@robmen
robmen Mar 10, 2015 Member

I think _CACHE should go before _INSTALL here since that matches the order of the enum better.

@heaths
heaths May 9, 2015 Contributor

Actually, seems it should be its own switch case since we'd want the default requested state to be BOOTSTRAPPER_REQUEST_STATE_CACHE. Fixed.

@robmen robmen and 1 other commented on an outdated diff Mar 10, 2015
src/burn/engine/plan.cpp
@@ -575,7 +595,11 @@ extern "C" HRESULT PlanRegistration(
UINT cDependencies = 0;
pPlan->fRegister = TRUE; // register the bundle since we're modifying machine state.
- pPlan->fKeepRegistrationDefault = (pRegistration->fInstalled || BOOTSTRAPPER_RESUME_TYPE_REBOOT == resumeType); // keep the registration if the bundle was already installed or we are planning after a restart.
+
+ // Keep the registration if the bundle was already installed or we are planning after a restart.
+ // Also keep the registration if we are caching because the ARP (via uninstall) allows uncaching.
+ pPlan->fKeepRegistrationDefault = (pRegistration->fInstalled || BOOTSTRAPPER_RESUME_TYPE_REBOOT == resumeType || BOOTSTRAPPER_ACTION_CACHE == pPlan->action);
@robmen
robmen Mar 10, 2015 Member

This doesn't seem right. This will keep the bundle registered even if _CACHE fails and does rollback.

@heaths
heaths Mar 12, 2015 Contributor

Good point. We would need to fix if now required.

@heaths
heaths May 9, 2015 Contributor

Looking through this further, I think this was originally since caching was a background process (which I also removed). Perhaps something to reconsider if/when we add async caching back in.

@robmen robmen and 2 others commented on an outdated diff Mar 10, 2015
src/burn/engine/plan.cpp
@@ -853,6 +877,11 @@ static HRESULT ProcessPackage(
hr = PlanLayoutPackage(pPlan, pPackage, wzLayoutDirectory);
ExitOnFailure(hr, "Failed to plan layout package.");
}
+ else if (BOOTSTRAPPER_ACTION_CACHE == pPlan->action)
@robmen
robmen Mar 10, 2015 Member

This doesn't seem right either. This will prevent a BA from deciding to do a _CACHE then set on of the packages as "Installed". _CACHE should set the default request state to cache and let the rest of the code flow through (in case the BA changes the default request state).,

@heaths
heaths Mar 12, 2015 Contributor

Not sure I follow. You're saying that if BURN_PLAN::action is _CACHE, then just set the default package request state to _CACHE and let the rest flow through naturally?

@heaths
Contributor
heaths commented Mar 12, 2015

Since you took @rseanhall's implementation, is there any point in this PR now? I don't understand what he was getting at since he was claiming there was a bug in something that Burn didn't seem to support (which was waiting on the original PR that included async).

heaths added some commits May 9, 2015
@heaths heaths Merge branch 'develop' into issue4432
Conflicts:
	History.md
	src/burn/engine/plan.cpp
b2c8a08
@heaths heaths Support cache-only mode in Burn via BA
Resolves issue #4432 by adding a BOOTSTRAPPER_ACTION_CACHE mode that BAs can set (i.e. Burn does not directly
support setting via command line) and will plan to cache all package types by default.

WixStdBA was updated to support this mode if opted in via a new @SupportCacheOnly attribute.
62ddfa2
@rseanhall rseanhall and 1 other commented on an outdated diff May 19, 2015
src/burn/engine/plan.cpp
@@ -1920,6 +1927,10 @@ static HRESULT GetActionDefaultRequestState(
switch (action)
{
+ case BOOTSTRAPPER_ACTION_CACHE:
+ *pRequestState = BOOTSTRAPPER_REQUEST_STATE_CACHE;
@rseanhall
rseanhall May 19, 2015 Member

This needs to be like MODIFY, it depends on the current state of the package. If the package is PRESENT and the default action is CACHE, then the package would get uninstalled.

@heaths
heaths May 25, 2015 Contributor

Good point. I'll need to go back and check our internal code since this has changed a bit to see if there's a problem there. Looking through the current and request states, seems it's just (current->requested): present->present, cache->none (though I can't find anything in the code not commented out that sets BOOTSTRAPPER_PACKAGE_STATE_CACHE), and *->cache.

@rseanhall rseanhall commented on an outdated diff May 19, 2015
src/burn/engine/plan.cpp
@@ -2775,6 +2797,22 @@ static HRESULT PlanDependencyActions(
hr = DependencyPlanPackageBegin(fBundlePerMachine, pPackage, pPlan);
ExitOnFailure(hr, "Failed to begin plan dependency actions for package: %ls", pPackage->sczId);
+ // All packages that have cacheType set to always should be cached if the bundle is going to be present.
@rseanhall
rseanhall May 19, 2015 Member

I put this block of code in different places than your original pull request, adding it here will cause it to be done twice. The BURN_CACHE_TYPE_ALWAYS condition is in ProcessPackage and NeedsCache. AddCacheSlipstreamMsps is in AddCachePackage.

@rseanhall rseanhall commented on the diff May 19, 2015
src/burn/engine/plan.cpp
@@ -1033,6 +1037,9 @@ extern "C" HRESULT PlanCachePackage(
if (fBARequestedCache || NeedsCache(pPlan, pPackage))
{
+ // The behavior for cache only mode is to do nothing on rollback, e.g. for subsequent install on demand scenarios.
@rseanhall
rseanhall May 19, 2015 Member

Doesn't this make it possible for packages to be in the cache without the bundle available to clean it out?

@heaths
heaths May 25, 2015 Contributor

It's possible, yes, but part of the point. When we introduced this feature, it was to pre-cache the Windows Phone emulator (in the background, hence async originally coming in with this feature) so it's available when you want it later but may not be online. A bundle can still remove the cache even if its not installed, though.

@rseanhall
rseanhall May 25, 2015 Member

A bundle can remove the cache if it's not installed, but the user can't run the bundle unless it's available somewhere. It's not very user friendly for a package to be put in the package cache without also ensuring its owning bundle is also in the cache.

@heaths
heaths May 25, 2015 Contributor

Just about to update the PR. I'd recommend, if you want to discuss further, we bring it up at the next triage and file it as a feature bug. This needs to get checked in and merging grows difficult.

Besides, the bundle gets cached as well so it is somewhere.

@rseanhall rseanhall commented on the diff May 19, 2015
src/burn/engine/plan.cpp
@@ -2061,11 +2072,18 @@ static HRESULT AddCachePackageHelper(
pCacheAction->type = BURN_CACHE_ACTION_TYPE_CHECKPOINT;
pCacheAction->checkpoint.dwId = dwCheckpoint;
- hr = AppendRollbackCacheAction(pPlan, &pCacheAction);
- ExitOnFailure(hr, "Failed to append rollback cache action.");
+ // Only plan the cache rollback if the package is also going to be uninstalled;
+ // otherwise, future operations like repair will not be able to locate the cached package.
+ BOOL fPlanCacheRollback = (BOOTSTRAPPER_ACTION_STATE_UNINSTALL == pPackage->rollback);
@rseanhall
rseanhall May 19, 2015 Member

Doesn't this make it possible for packages to be in the cache without the bundle available to clean it out?

heaths added some commits May 25, 2015
@heaths heaths Merge branch 'develop' into issue4432
Conflicts:
	History.md
	src/ext/BalExtension/wixext/BalCompiler.cs
	src/ext/BalExtension/wixext/Xsd/bal.xsd
	src/ext/BalExtension/wixext/data/tables.xml
	src/ext/BalExtension/wixstdba/WixStandardBootstrapperApplication.cpp
a811877
@heaths heaths Remove now-duplicate code and other review feedback 2e02aeb
@rseanhall
Member

This looks like it's ready to merge (except for the History.md changes that are easily fixed). I'll have to test out the failure scenario and see what get left behind in the cache.

@barnson barnson merged commit e0afc83 into wixtoolset:develop Jun 7, 2015
@heaths heaths deleted the heaths:issue4432 branch Jun 7, 2015
@wixbot wixbot referenced this pull request in wixtoolset/issues Dec 20, 2015
Closed

Add burn support for only caching packages #4432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment