[wix-devs] OnExecutePackageComplete without OnExecutePackageBegin

Thu Mar 17 10:32:46 PDT 2016

All the Release* macros do that, just using NULL or some other value instead of an extra BOOL. If anything, the "do real work in LExit" is the unusual case. 

> -----Original Message-----
> From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On Behalf Of
> Sean Hall
> Sent: Thursday, 17 March, 2016 13:28
> To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> Subject: Re: [wix-devs] OnExecutePackageComplete without
> OnExecutePackageBegin
> 
> I believe there is already precedence in burn to have a boolean initially set to
> false, then set to true once something happened.  In LExit, it then performs
> additional work if it was set to true.
> 
> On Thu, Mar 17, 2016 at 12:19 PM, Hoover, Jacob
> <Jacob.Hoover at greenheck.com>
> wrote:
> 
> > Putting that in is bad, but if we HAVE to do it...  I would envision
> > an ExitEarlyWithSuccess macro. The real issue is going to be not
> > jumping to LExit where all the cleanup is guaranteed to happen.
> >
> > -----Original Message-----
> > From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On
> > Behalf Of Heath Stewart
> > Sent: Thursday, March 17, 2016 12:11 PM
> > To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> > Subject: Re: [wix-devs] OnExecutePackageComplete without
> > OnExecutePackageBegin
> >
> > So how do people - Rob and Bob in particular - feel about an early
> > return (i.e. "return S_OK") in this case? Since this may, in fact, set
> > a precedent I'd like to know what's acceptable and I can code up a fix.
> >
> > Thanks,
> >
> > Heath Stewart
> > Visual Studio, Microsoft
> > http://blogs.msdn.com/heaths
> >
> >
> > -----Original Message-----
> > From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On
> > Behalf Of Heath Stewart
> > Sent: Wednesday, March 16, 2016 1:45 PM
> > To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> > Subject: Re: [wix-devs] OnExecutePackageComplete without
> > OnExecutePackageBegin
> >
> > OnCache* is fine. I was using that as an example of what is, IMO,
> > coded correctly. But when package execution begins it checks the cache
> > result and bails before telling the BA package execution started
> > (which is actually in each engine's execute functions), but the
> > general LExit block calls OnExecutePackageComplete. That's all in the code.
> >
> > From Rob, I'll file a bug and get a PR out, but proposing a true early
> > termination via return.
> >
> > Here's the gist of the code:
> >
> > Execute*Package(...) {
> >   if (pPackage->hrCacheResult) {
> >     Log();
> >     ExitFunction1(hr = S_OK); // propose to just return S_OK;
> >   }
> >   ba->OnPackageExecuteBegin(...);
> >   // other stuff, all which often uses Exit*() macros
> > LExit:
> >   hr = ExecutePackageComplete(...); // calls ba-
> >OnPackageExecuteComplete
> >   return hr;
> > }
> >
> > It makes sense we bail from execute() if caching failed (no package on
> > which to execute, in most cases anyway) but execute() was already
> > scheduled. The runtime couldn't have known (practically) that caching
> > would fail, so the extra check makes sense. But calling the
> > ExitFunction macro ends up calling OnExecutePackageComplete, so we
> > either return early or maybe skip to another label like LExit2 after
> > LExit, in which case any future general cleanup could be done. I don't
> > see any precedent so curious what others think.
> >
> > Heath Stewart
> > Visual Studio, Microsoft
> >
> > https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fblogs.
> >
> msdn.com%2fheaths&data=01%7c01%7cHeath.Stewart%40microsoft.com%7
> cf1621
> >
> d6cadb1448b3ad308d34ddbcd82%7c72f988bf86f141af91ab2d7cd011db47%7c
> 1&sda
> > ta=JBWCYu5g%2blw0tqTjz6qZDqQdDeTh3j21if%2bQ95AT8XQ%3d
> >
> > -----Original Message-----
> > From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On
> > Behalf Of Sean Hall
> > Sent: Wednesday, March 16, 2016 10:10 AM
> > To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> > Subject: Re: [wix-devs] OnExecutePackageComplete without
> > OnExecutePackageBegin
> >
> > I think every Begin callback should always have a corresponding
> > Complete callback, even if an error occurred.  A Complete callback
> > should never be sent unless the corresponding Begin callback was sent,
> > if Burn is doing this then I think that's a bug.
> >
> > I'd have to look at code for the OnCacheBegin/OnCacheComplete
> > question, if the whole cache phase is skipped I don't think they should be
> sent.
> >
> > On Wed, Mar 16, 2016 at 12:03 PM, Heath Stewart <
> > Heath.Stewart at microsoft.com
> > > wrote:
> >
> > > We ran into a scenario where a caching error prevented
> > > OnExecutePackageBegin from executing (basically, in apply.cpp, the
> > > check for the caching HRESULT happens just before calling
> > > OnExecutePackageBegin on the BA) but OnExecutePackageComplete
> was
> > > called (it's the first line after the LExit label for each
> > > package-specific execute function in apply.cpp. We can work around
> > > this (just keep track of what packages were started and check that
> > > set in the completion callback), but begs the question whether in
> > > burn we should call a Complete callback without a corresponding
> > > Begin callback. It's not common to see that sort of behavior
> > > (thinking about
> > async APIs in various frameworks I've used).
> > >
> > > What is more consistent with typical async behavior - and allows a
> > > BA to clean anything specific to the downloading/caching of a
> > > package still - is that ApplyCache does call OnCacheBegin invariably
> > > (i.e. no exits before said call) and OnCacheComplete in the LExit
> > > (again with no early exits) so they are always a pair.
> > >
> > > So should this maybe changed for wix4?
> > >
> > > Heath Stewart
> > > Visual Studio, Microsoft
> > >
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fblogs.
> > >
> msdn.com%2fheaths&data=01%7c01%7cHeath.Stewart%40microsoft.com%7
> cebe
> > > 5c
> > >
> 84c7e9141ecfa8308d34dbdd44d%7c72f988bf86f141af91ab2d7cd011db47%7c1
> &s
> > > da
> ta=E%2bPWtOXEyuBU3%2bx7%2fZbPH50OTH1hwEOMy1WfFptWFek%3d
> > >
> > >
> __________________________________________________________
> __________
> > > WiX Toolset Developer Mailing List provided by FireGiant
> > >
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.
> > > fi
> > >
> regiant.com%2f&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cebe
> 5c8
> > > 4c
> > >
> 7e9141ecfa8308d34dbdd44d%7c72f988bf86f141af91ab2d7cd011db47%7c1&s
> dat
> > > a= AYTU4NnSFIqWFLIZvQqVPQPcma7lhgwNr6unMJBA0XI%3d
> > >
> >
> __________________________________________________________
> __________
> > WiX Toolset Developer Mailing List provided by FireGiant
> >
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.fi
> >
> regiant.com%2f&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cebe
> 5c84c
> >
> 7e9141ecfa8308d34dbdd44d%7c72f988bf86f141af91ab2d7cd011db47%7c1&s
> data=
> > AYTU4NnSFIqWFLIZvQqVPQPcma7lhgwNr6unMJBA0XI%3d
> >
> __________________________________________________________
> __________
> > WiX Toolset Developer Mailing List provided by FireGiant
> >
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.fi
> >
> regiant.com%2f&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cf162
> 1d6c
> >
> adb1448b3ad308d34ddbcd82%7c72f988bf86f141af91ab2d7cd011db47%7c1&s
> data=
> > znjVw8QcKUThgGCvUdXkqfvUAO4fL3RNk4iZ6DGzbV0%3d
> >
> __________________________________________________________
> __________
> > WiX Toolset Developer Mailing List provided by FireGiant
> > http://www.firegiant.com/
> >
> __________________________________________________________
> __________
> > WiX Toolset Developer Mailing List provided by FireGiant
> > http://www.firegiant.com/
> >
> __________________________________________________________
> __________
> WiX Toolset Developer Mailing List provided by FireGiant
> http://www.firegiant.com/