[wix-devs] #3640 - Burn will use incorrect payload if local

Fri Apr 9 09:40:05 PDT 2021

1. Heh. Sure. *Much* faster. ;)

2. Yeah, I was thinking a OnCacheAcquireResolving() call when it's args.fFoundLocal==FALSE is the closest thing to OnResolveSource(), right? Is it the case where the file was not found locally so it will either be downloaded (if available) or fail that the file cannot be found (since a new source cannot provided)?

3,4. I think prompting for source in OnCacheAcquireComplete() then returning retry should work out just fine. It might even be better.

-----Original Message-----
From: wix-devs <wix-devs-bounces at lists.wixtoolset.org> On Behalf Of Sean Hall via wix-devs
Sent: Thursday, April 8, 2021 12:46 PM
To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
Cc: Sean Hall <r.sean.hall at gmail.com>
Subject: Re: [wix-devs] #3640 - Burn will use incorrect payload if local

1. I didn't measure it. It's one less API call per file, so it has to be faster :)

>  If I understand correctly, OnResolveSource is essentially replaced by
OnCacheAcquireResolving when args.fFoundLocal == FALSE

2. No, OnCacheAcquireResolving is called for every payload that is being acquired. If the BA wants to block the download, it can change the action to NONE from OnCacheAcquireResolving or it can specify COPY from OnCacheAcquireBegin.

>  Most (all?) BAs prompt the user for source during OnResolveSource

3. That's probably because that was the only time during Apply that the engine would allow the BA to modify its state :). I understand that it would be ideal if the BA could set source from OnCacheAcquireResolving but like I said, we need to solve the string problem for that. The main problem here is the local source string, we could probably allow setting the download source but there's not currently a way to lock down the engine like that.

4. I do think that with the current state of the PR, the BA would prompt for source on failure from OnCacheAcquireComplete and then retry.

On Thu, Apr 8, 2021 at 2:11 PM Rob Mensching <rob at firegiant.com> wrote:

> These are great problems to get solved in Burn.
>
>
> > During Detect, count a package as cached if any of a package's 
> > payloads
> exist without checking the file size. This makes Detect faster.
>
> 1. Just curious: Is it noticeably faster? I thought checking file size 
> would be a simple NTFS directory table lookup. It must be a thing when 
> there are many, many, many loose payloads, right?
>
>
> > then the engine will now automatically try to download it if it has 
> > a
> DownloadUrl
>
> 2. FYI, this was a (probably undocumented) requirement that the BA was 
> always called before the engine would go to the internet. That 
> requirement came from the early design days of Burn when internet 
> downloads were more costly/less prolific so it's probably okay to toss 
> it. I just wanted to raise it in case it informed/changed anything.
>
>
> > It is not allowed in the new OnCacheAcquireResolving event, but only
> because we don't have a good way for the BA to send strings to the 
> engine to interact with a BA event.
>
> 3. Most (all?) BAs prompt the user for source during OnResolveSource. 
> If I understand correctly, OnResolveSource is essentially replaced by 
> OnCacheAcquireResolving when args.fFoundLocal == FALSE. If so, that's 
> the most likely case where set source would be called (after the user 
> was prompted for the new source) and thus I think we should make that 
> possible and as easy as possible. Unless the correct solution is to 
> use OnCacheAcquireComplete, if so, see the next point.
>
>
> > If the web server requires authentication, the BA could set that for
> every payload in OnCacheAcquireBegin
>
> 4. Similar to the above issue, the BA is likely to prompt the user for 
> credentials on demand (in this case after fail to get a file due to 
> authentication required). If the solution is to handle failure in 
> OnCacheAcquireComplete by setting the source credentials then 
> retrying, I expect that's cool.
>
>
> -----Original Message-----
> From: wix-devs <wix-devs-bounces at lists.wixtoolset.org> On Behalf Of 
> Sean Hall via wix-devs
> Sent: Tuesday, April 6, 2021 12:59 PM
> To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> Cc: Sean Hall <r.sean.hall at gmail.com>
> Subject: [wix-devs] #3640 - Burn will use incorrect payload if local
>
> I have implemented a solution for this at 
> https://github.com/wixtoolset/burn/pull/48. There's still some work 
> left to get the progress right. This pull request is fixing some design flaws:
>
> During Detect, a payload was assumed as cached if the file existed and 
> had the right size. In v3 and v4, this could have been a false 
> positive if it had the wrong hash. In v3, this could have been a false 
> negative if the payload was supposed to be verified through 
> Authenticode. This partial verification was done because full 
> verification could make Detect take too long.
>
> When trying to acquire a payload and the engine found a suitable file 
> in one of its probe paths, the BA never got a chance to change the 
> source even if the payload verification failed.
>
> The engine made it impossible for the BA to request that an embedded 
> be downloaded instead of acquired through the container (#5253).
>
>
> The fixes for these flaws are:
>
> During Detect, count a package as cached if any of a package's 
> payloads exist without checking the file size. This makes Detect faster.
>
> Cache planning is done at the package level instead of the payload 
> level, which ensures that all payloads for packages being cached go 
> through full verification. This also allows the BA to request that an 
> embedded payload is downloaded instead of extracted from a container.
>
> The BA can set the source during the OnCacheAcquireBegin/Complete 
> events, which are always sent to the BA if the payload wasn't already 
> present and verified.
>
>
> The API has changed between the engine and the BA, full details can be 
> seen in the PR. The OnResolveSource event has been removed. This event 
> was basically doing two things: allowing the BA to request that the BA 
> be downloaded and allowing the BA to set the source. A new 
> OnCacheAcquireResolving event has been added.
>
> For the request to download, this should be needed rarely after this 
> pull request. If the payload can't be found locally and isn't embedded 
> in a container, then the engine will now automatically try to download 
> it if it has a DownloadUrl. That being said, the BA can request to 
> download from OnCacheAcquireBegin or the new OnCacheAcquireResolving event.
>
> For setting the source, this is now allowed in OnCacheAcquireBegin and 
> OnCacheAcquireComplete. It is not allowed in the new 
> OnCacheAcquireResolving event, but only because we don't have a good 
> way for the BA to send strings to the engine to interact with a BA event.
>
> Some examples:
>
> 1. Uncompressed bundle, web download
>
> In this scenario, the user downloads the bundle .exe by itself and 
> everything is downloaded as needed. All payloads have a DownloadUrl. 
> In v3, the BA would have to request Download in OnResolveSource for every payload.
> With this PR, no BA code is required because the engine will 
> automatically try to use the DownloadUrl. If the web server requires 
> authentication, the BA could set that for every payload in 
> OnCacheAcquireBegin or like v3 could just use the BA manifest to go 
> through all the payloads and set them before Apply.
>
> 2. Uncompressed bundle, DVD layout
>
> In this scenario, the user runs the bundle .exe from a DVD that has 
> all the payloads next to it. There should be no change in behavior 
> here, the engine should find all payloads next to the bundle with no BA code required.
>
> 3. Fully compressed bundle
>
> In this scenario, the bundle was installed. The user modifies the 
> installation and installs a package not already present. In v3, the 
> only way to get the required payload would be to provide the original bundle.
> With this PR, the BA could set the DownloadUrl during 
> OnCacheAcquireBegin and request Download to avoid the engine trying to 
> locate the original bundle. There would be a corresponding change in 
> Core that allows the authoring of a DownloadUrl for embedded payloads 
> so that the BA would only have to request download during 
> OnCacheAcquireBegin or OnCacheAcquireResolving.
>
> An aside:
>
> The .NET Core team solves the string problem by providing callbacks. 
> For example, their new API that they added to list the runtimes and 
> SDKs on the
> machine: https://github.com/dotnet/runtime/issues/46128.
>
> For these events where we want to allow changing the source, we could 
> pass in additional parameters that are callback functions. For example:
>
> struct BA_ONCACHEACQUIRERESOLVING_ARGS {
>     PFN_BOOTSTRAPPER_ENGINE_SETLOCALSOURCE pfnSetLocalSource;
>     LPVOID pvContext;
> }
>
> extern "C" typedef HRESULT(WINAPI *PFN_BOOTSTRAPPER_ENGINE_SETLOCALSOURCE)(
>     __in LPCWSTR wzPath,
>     __in LPVOID pvContext
>     );
>
> To keep the binary compatibility goal, it would probably have to 
> instead use the ARGS and RESULTS struct paradigm but this is easier to write.
> ____________________________________________________________________
> WiX Toolset Developer Mailing List provided by FireGiant 
> http://www.firegiant.com/
>
____________________________________________________________________
WiX Toolset Developer Mailing List provided by FireGiant http://www.firegiant.com/