New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wixfeat4190.1 #29

Merged
merged 4 commits into from Jun 12, 2014

Conversation

Projects
None yet
3 participants
@jchoover
Contributor

jchoover commented May 8, 2014

I think I finally have the right branch merged and in the right state for this to function.

These are the needed 3.x changes for a BA/UX to be able to self update. In 4.x, it's desirable to allow OnDetectUpdateComplete to modify the HRESULT returned by DetectUpdate (so if a user is offline or the website is down, that the BA doesn't have to call Detect a second time).

// If the destination file already exists, clear the readonly bit to avoid E_ACCESSDENIED.
if (FileExistsEx(sczDestinationPath, &dwFileAttributes))

This comment has been minimized.

@barnson

barnson May 9, 2014

Member

Don't think this can happen (from PathCreateTimeBasedTempFile).

@barnson

barnson May 9, 2014

Member

Don't think this can happen (from PathCreateTimeBasedTempFile).

dwFileAttributes &= ~FILE_ATTRIBUTE_READONLY;
if (!::SetFileAttributesW(sczDestinationPath, dwFileAttributes))
{
ExitWithLastError1(hr, "Failed to clear readonly bit on payload destination path: %ls", sczDestinationPath);

This comment has been minimized.

@barnson

barnson May 9, 2014

Member

I smell a copy/paste... :)

@barnson

barnson May 9, 2014

Member

I smell a copy/paste... :)

// IDCANCEL instructs the engine to stop detection.
//
// IDNOACTION instructs the engine to process further update candidates.
STDMETHOD_(int, OnDetectUpdate)(

This comment has been minimized.

@barnson

barnson May 10, 2014

Member

Any chance these could be NULL?

@barnson

barnson May 10, 2014

Member

Any chance these could be NULL?

@rseanhall

This comment has been minimized.

Show comment
Hide comment
@rseanhall

rseanhall May 10, 2014

Member
  1. To address having to call Detect twice, I would add a Begin/Complete
    pair of methods to IBootstrapperApplication around lines 412-422 of
    detect.cpp (where you call
    AtomInitialize/DownloadUpdateFeed/AtomParseFromFile/ApupAllocChainFromAtom).
    This would allow the BA to tell the engine to ignore errors from those
    methods. If you give the path to the temp file, it could also be a way to
    give the BA access to the downloaded update feed in case it's not in the
    format the engine understands.
  2. Does Detect and Apply really need their own AuthenticationRequired
    methods? What's the guideline for duplicating code like this?
  3. For the OnDetectPackageBegin method, the engine doesn't call the
    OnDetectPackageComplete if there's an error while processing the package.
    But it always calls OnDetectUpdateComplete if it calls
    OnDetectUpdateBegin. Shouldn't it be consistent? (Jacob's pull request
    didn't alter this behavior, it was there already)
  4. Bob made a comment wondering whether the parameters in the new
    OnDetectUpdate method can be null. Looking through apuputil.cpp, it looks
    like the only thing guaranteed to be set in APPLICATION_UPDATE_ENTRY is
    dw64Version. This means that not only can most of the parameters be null,
    the engine will crash if there's no enclosures. I think there needs to be
    a spec for the feed that the engine expects, and it needs to be documented
    in the manual and enforced in the code.
  5. The new DetectUpdateEventArgs class inherits from the
    DetectUpdateBeginEventArgs. This inheritance doesn't make sense to me.
    Thoughts?

On Thu, May 8, 2014 at 3:49 PM, jchoover notifications@github.com wrote:

I think I finally have the right branch merged and in the right state for
this to function.

These are the needed 3.x changes for a BA/UX to be able to self update. In
4.x, it's desirable to allow OnDetectUpdateComplete to modify the HRESULT
returned by DetectUpdate (so if a user is offline or the website is down,

that the BA doesn't have to call Detect a second time).

You can merge this Pull Request by running

git pull https://github.com/jchoover/wix3 WIXFEAT4190.1

Or view, comment on, or merge it at:

#29

Member

rseanhall commented May 10, 2014

  1. To address having to call Detect twice, I would add a Begin/Complete
    pair of methods to IBootstrapperApplication around lines 412-422 of
    detect.cpp (where you call
    AtomInitialize/DownloadUpdateFeed/AtomParseFromFile/ApupAllocChainFromAtom).
    This would allow the BA to tell the engine to ignore errors from those
    methods. If you give the path to the temp file, it could also be a way to
    give the BA access to the downloaded update feed in case it's not in the
    format the engine understands.
  2. Does Detect and Apply really need their own AuthenticationRequired
    methods? What's the guideline for duplicating code like this?
  3. For the OnDetectPackageBegin method, the engine doesn't call the
    OnDetectPackageComplete if there's an error while processing the package.
    But it always calls OnDetectUpdateComplete if it calls
    OnDetectUpdateBegin. Shouldn't it be consistent? (Jacob's pull request
    didn't alter this behavior, it was there already)
  4. Bob made a comment wondering whether the parameters in the new
    OnDetectUpdate method can be null. Looking through apuputil.cpp, it looks
    like the only thing guaranteed to be set in APPLICATION_UPDATE_ENTRY is
    dw64Version. This means that not only can most of the parameters be null,
    the engine will crash if there's no enclosures. I think there needs to be
    a spec for the feed that the engine expects, and it needs to be documented
    in the manual and enforced in the code.
  5. The new DetectUpdateEventArgs class inherits from the
    DetectUpdateBeginEventArgs. This inheritance doesn't make sense to me.
    Thoughts?

On Thu, May 8, 2014 at 3:49 PM, jchoover notifications@github.com wrote:

I think I finally have the right branch merged and in the right state for
this to function.

These are the needed 3.x changes for a BA/UX to be able to self update. In
4.x, it's desirable to allow OnDetectUpdateComplete to modify the HRESULT
returned by DetectUpdate (so if a user is offline or the website is down,

that the BA doesn't have to call Detect a second time).

You can merge this Pull Request by running

git pull https://github.com/jchoover/wix3 WIXFEAT4190.1

Or view, comment on, or merge it at:

#29

if (IDOK == nResult)
{
break;

This comment has been minimized.

@rseanhall

rseanhall May 10, 2014

Member

I think you need to put "pUpdate->fUpdateAvailable = TRUE;" here.

@rseanhall

rseanhall May 10, 2014

Member

I think you need to put "pUpdate->fUpdateAvailable = TRUE;" here.

@@ -54,6 +54,7 @@ enum BOOTSTRAPPER_ERROR_TYPE
BOOTSTRAPPER_ERROR_TYPE_HTTP_AUTH_SERVER, // error occurred trying to authenticate with HTTP server.
BOOTSTRAPPER_ERROR_TYPE_HTTP_AUTH_PROXY, // error occurred trying to authenticate with HTTP proxy.
BOOTSTRAPPER_ERROR_TYPE_APPLY, // error occurred during apply.
BOOTSTRAPPER_ERROR_TYPE_UPDATE, // error occurred during updating.

This comment has been minimized.

@rseanhall

rseanhall May 10, 2014

Member

This is never used as far as I can tell.

@rseanhall

rseanhall May 10, 2014

Member

This is never used as far as I can tell.

@@ -32,6 +32,7 @@ enum BOOTSTRAPPER_ACTION
BOOTSTRAPPER_ACTION_REPAIR,
BOOTSTRAPPER_ACTION_UPDATE_REPLACE,
BOOTSTRAPPER_ACTION_UPDATE_REPLACE_EMBEDDED,
BOOTSTRAPPER_ACTION_UPDATE_NONE,

This comment has been minimized.

@rseanhall

rseanhall May 10, 2014

Member

This is never used as far as I can tell.

@rseanhall

rseanhall May 10, 2014

Member

This is never used as far as I can tell.

barnson added a commit that referenced this pull request Jun 12, 2014

@barnson barnson merged commit 7a171aa into wixtoolset:develop Jun 12, 2014

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