Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

win,msi: broadcast WM_SETTINGCHANGE after install #25100

Closed
wants to merge 2 commits into from
Closed

win,msi: broadcast WM_SETTINGCHANGE after install #25100

wants to merge 2 commits into from

Conversation

orangemocha
Copy link
Contributor

Fix for #4356

The second commit is pulled verbatim from nodejs/node@668bde8

The first commit is the delta from io.js, required to make the other commit merge cleanly.

Opening PR against v0.10 first.

@orangemocha
Copy link
Contributor Author

cc @joyent/node-coreteam

@misterdjules
Copy link

LGTM, @orangemocha did you have some time to test and verify that it fixes the problem?

@orangemocha
Copy link
Contributor Author

We tested it and verified that it sends WM_SETTINGCHANGE after this change. However, we were not able to reproduce the original issue, much like @piscisaureus mentioned in nodejs/node#603. But a few people have reported the issue, so we have to trust that it can happen on some systems.

@misterdjules
Copy link

@orangemocha OK, sounds good.

Regarding the commit message for the backported commit, maybe we should format it like:

Backport [sha1] from io.js.

original commit message:
  The full commit mesage.

So that it doesn't become very confusing when the CI adds the PR-URL, the reviewer and other metadata from joyent/node.

Also, this PR is targeted to the v0.10 branch but is in the 0.12.3 milestone, so I suggest moving it to the 0.10.39 milestone.

joaocgreis and others added 2 commits May 8, 2015 12:28
This is needed so that we can backport
668bde8 from io.js with
a clean merge.
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 <bertbelder@gmail.com>
(cherry picked from commit 668bde8)

Conflicts:
	tools/msvs/msi/product.wxs
@orangemocha orangemocha modified the milestones: 0.10.39, 0.12.3 May 8, 2015
@orangemocha
Copy link
Contributor Author

Thanks @misterdjules! I updated both commits' messages and changed the milestone to 0.10.39.

orangemocha pushed a commit that referenced this pull request May 11, 2015
This is needed so that we can backport
668bde8 from io.js with
a clean merge.

PR-URL: #25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: #4356
orangemocha pushed a commit that referenced this pull request May 11, 2015
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 <bertbelder@gmail.com>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: #25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: #4356
@orangemocha
Copy link
Contributor Author

Landed in ad9947e and e7c84f8.

@orangemocha
Copy link
Contributor Author

@misterdjules will you be merging v0.10 into v0.12 before this week's release? If not I can do it. Thanks!

@misterdjules
Copy link

@orangemocha I'd like to determine first what we're doing with the RC4 deprecation fixes (see #14572 for example) before merging v0.10 in v0.12. If these fixes are not ready, then we might revert them in the v0.10 and v0.12 branches, and only then merge v0.10 into v0.12.

Also, did you use git-apply-pr to land these commits? If not, that's the recommended way to do it in v0.10, it makes the metadata format consistent for all changes. I think it's probably not documented in our contributing guidelines though, we should probably fix that.

@orangemocha
Copy link
Contributor Author

@misterdjules Ok.

I landed these commits manually. We could also just mark the failing unit tests as flaky and use node-accept-pull-request on v0,10 as well

misterdjules pushed a commit to misterdjules/node that referenced this pull request May 11, 2015
This is needed so that we can backport
668bde8 from io.js with
a clean merge.

PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: nodejs#4356
misterdjules pushed a commit to misterdjules/node that referenced this pull request May 11, 2015
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 <bertbelder@gmail.com>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: nodejs#4356
@misterdjules
Copy link

@orangemocha Yes, that would probably be a good idea. We would also want to file an issue for each of the flakey tests so that we can track and fix them.

misterdjules pushed a commit to misterdjules/node that referenced this pull request May 13, 2015
This is needed so that we can backport
668bde8 from io.js with
a clean merge.

PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: nodejs#4356
misterdjules pushed a commit to misterdjules/node that referenced this pull request May 13, 2015
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 <bertbelder@gmail.com>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: nodejs#4356
@misterdjules misterdjules modified the milestones: 0.12.4, 0.10.39 May 13, 2015
@misterdjules misterdjules reopened this May 13, 2015
@misterdjules
Copy link

We can't really merge v0.10 and v0.12 now because of the recent changes in default ciphers list that still need some work before they are ready to ship, and I'd like to release v0.12.3 asap.

We could cherry pick the changes on v0.12, but that's a bit last minute and it's not a very severe issue.

So I made the decision to wait for v0.10.39 to be ready to merge this into v0.12, in the meantime I'm reopening the PR so that we can track that this needs to be integrated for node v0.12.4.

/cc @joyent/node-coreteam

@orangemocha
Copy link
Contributor Author

That sounds reasonable to me, especially considered that it's a P-3 issue. Thanks @misterdjules !

@misterdjules misterdjules modified the milestones: 0.12.4, 0.12.5 May 25, 2015
@misterdjules
Copy link

Done, merged in v0.12 and shipped with node v0.12.5. Thank you!

@orangemocha
Copy link
Contributor Author

Great, thanks @misterdjules 👍

jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
This is needed so that we can backport
668bde8 from io.js with
a clean merge.

PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: nodejs#4356
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
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 <bertbelder@gmail.com>
(cherry picked from commit 668bde8)

--Node.js commmit metadata--
PR-URL: nodejs#25100
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Fixes: nodejs#4356
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants