Skip to content
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.

Sign up for GitHub

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

Jump to bottom

msi: Add WM_SETTINGCHANGE broadcast on InstallFinalize #613

Closed
wants to merge 3 commits into from
Closed

msi: Add WM_SETTINGCHANGE broadcast on InstallFinalize #613

wants to merge 3 commits into from

Conversation

mathiask88
Copy link
Contributor

Fix: #603

@rvagg
Copy link
Member

rvagg commented Jan 26, 2015

lgtm, I'll make sure this gets in a nightly later today, I'll merge if @piscisaureus doesn't first

@piscisaureus
Copy link
Contributor

Looks good enough for now, let's get this into a nightly and then a release. I want to see if this fixes the issue.

For later reference, there are some things I'd do to reduce this patch

  • Drop the precompiled header and targetver.h
  • The .res file doesn't need to be checked in.
  • I prefer not to use msvs-specific __in qualifiers on function signatures.
  • I prefer not to use msvs-specific #pragma directives.

@piscisaureus
Copy link
Contributor

I've reduced the patch a little: https://github.com/piscisaureus/io.js/compare/msi-broadcast-setting-change.

@mathiask88
Copy link
Contributor Author

@piscisaureus Did you test the patch? It compiles but the msi prompts with an error A DLL required for this install to complete could not be run.

@mathiask88
Copy link
Contributor Author

I reduced the patch according to your hints. But I left the DLL export with the DEF file.

@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

given the big diff between these two proposals I'm not going to step in and merge this, will leave it to @piscisaureus to merge something and then we'll get that into a nightly

@piscisaureus
Copy link
Contributor

@mathiask88

@mathiask88
Copy link
Contributor Author

I am still going to call the file to custom_actions; I don't think there's any good reason to switch to camelcase naming.

Sure, as you want. I only chose that name because most of the WIX extensions are in camelcase.

Since you did most of the work, I'm going to make you the patch author if that's okay with you. Do you have a last name?

Oh, never noticed my incomplete github profile. Update is out ;)

piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Jan 27, 2015
@mathiask88 @piscisaureus
win,msi: broadcast WM_SETTINGCHANGE after install
668bde8 
In theory the msi should broadcast a 'WM_SETTINGCHANGE' message to all
windows after modifying the PATH environment variable. This ensures that
the new PATH is visible to other processes without restarting windows
(although it's still necessary to close and reopen active console
windows).

Unfortunately, the broadcast doesn't always happen, for unknown reasons.
That's why this patch adds a custom action that unconditionally
broadcasts a WM_SETTINGCHANGE message.

Bug: nodejs#603
PR: nodejs#613
Reviewed-by: Bert Belder <[email protected]>
@piscisaureus
Copy link
Contributor

@mathiask88
Ok, thanks. I'm running some final tests, will land after that if it all works.

@piscisaureus
Copy link
Contributor

Landed in 668bde8. @mathiask88 Thanks for your help!

orangemocha pushed a commit to nodejs/node-v0.x-archive that referenced this pull request May 11, 2015
@mathiask88 @orangemocha
win,msi: broadcast WM_SETTINGCHANGE after install
e7c84f8 
Backport 668bde8 from io.js.
Original commit message follows:

In theory the msi should broadcast a 'WM_SETTINGCHANGE' message to all
windows after modifying the PATH environment variable. This ensures that
the new PATH is visible to other processes without restarting windows
(although it's still necessary to close and reopen active console
windows).

Unfortunately, the broadcast doesn't always happen, for unknown reasons.
That's why this patch adds a custom action that unconditionally
broadcasts a WM_SETTINGCHANGE message.

Bug: nodejs/node#603
PR: nodejs/node#613
Reviewed-by: Bert Belder <[email protected]>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: #25100
Reviewed-By: Julien Gilli <[email protected]>
Fixes: #4356
misterdjules pushed a commit to misterdjules/node that referenced this pull request May 11, 2015
@mathiask88
win,msi: broadcast WM_SETTINGCHANGE after install
7935cfc 
Backport 668bde8 from io.js.
Original commit message follows:

In theory the msi should broadcast a 'WM_SETTINGCHANGE' message to all
windows after modifying the PATH environment variable. This ensures that
the new PATH is visible to other processes without restarting windows
(although it's still necessary to close and reopen active console
windows).

Unfortunately, the broadcast doesn't always happen, for unknown reasons.
That's why this patch adds a custom action that unconditionally
broadcasts a WM_SETTINGCHANGE message.

Bug: nodejs/node#603
PR: nodejs/node#613
Reviewed-by: Bert Belder <[email protected]>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <[email protected]>
Fixes: nodejs#4356
misterdjules pushed a commit to misterdjules/node that referenced this pull request May 13, 2015
@mathiask88
win,msi: broadcast WM_SETTINGCHANGE after install
ed4b046 
Backport 668bde8 from io.js.
Original commit message follows:

In theory the msi should broadcast a 'WM_SETTINGCHANGE' message to all
windows after modifying the PATH environment variable. This ensures that
the new PATH is visible to other processes without restarting windows
(although it's still necessary to close and reopen active console
windows).

Unfortunately, the broadcast doesn't always happen, for unknown reasons.
That's why this patch adds a custom action that unconditionally
broadcasts a WM_SETTINGCHANGE message.

Bug: nodejs/node#603
PR: nodejs/node#613
Reviewed-by: Bert Belder <[email protected]>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <[email protected]>
Fixes: nodejs#4356
@JacksonTian
Copy link
Contributor

When i build io.js with vs 2013, get error message:

wcautil.h no such file.

@piscisaureus
Copy link
Contributor

When i build io.js with vs 2013, get error message:

wcautil.h no such file.

I think that's unrelated. IO.js is normally built with vs2013 which (obviously) works.
wcautil.h is normally distributed with the WiX toolkit so your copy is likely outdated or broken.

@JacksonTian
Copy link
Contributor

I used wix 3.8, not find the wcautil.h. Update to 3.9, the file existed. I will retry.

@misterdjules
Copy link

@JacksonTian See nodejs/node-v0.x-archive#25569 for the description of a similar issue in joyent/node, and a work in progress to make such failures easier to troubleshoot.

jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
@mathiask88 @jBarz
win,msi: broadcast WM_SETTINGCHANGE after install
7811c5e 
Backport 668bde8 from io.js.
Original commit message follows:

In theory the msi should broadcast a 'WM_SETTINGCHANGE' message to all
windows after modifying the PATH environment variable. This ensures that
the new PATH is visible to other processes without restarting windows
(although it's still necessary to close and reopen active console
windows).

Unfortunately, the broadcast doesn't always happen, for unknown reasons.
That's why this patch adds a custom action that unconditionally
broadcasts a WM_SETTINGCHANGE message.

Bug: nodejs/node#603
PR: nodejs/node#613
Reviewed-by: Bert Belder <[email protected]>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <[email protected]>
Fixes: nodejs#4356
@johnsonj johnsonj mentioned this pull request Jan 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mathiask88 @rvagg @piscisaureus @JacksonTian @misterdjules