-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Windows post-install screen additional build tools messaging #4111
Conversation
This is fantastic! Great work @kaylynb. The documentation part is interesting, perhaps we should point to a Wiki page maintained at this repo and combine parts of the README with what's already in https://github.com/TooTallNate/node-gyp. The Microsoft page goes too far and our README isn't exactly suitable because it needs to be focused only on add-on compilation and nothing else. |
This is very good indeed! @kaylynb Did you try the instructions under "Customizing the ExitDlg" in http://wixtoolset.org/documentation/manual/v3/wixui/wixui_customizations.html ? I don't know if it can display links, but might be worth trying. @rvagg is there a way to have a redirect page somewhere that we control? That way we would never need to worry about dead links in already published installers. I think we can have the link point further down in that page, to https://github.com/Microsoft/nodejs-guidelines/blob/master/windows-environment.md#environment-setup-and-configuration . If we end up creating a wiki page, it would still be good to point to the nodejs-guidelines repo. |
@joaocgreis I did try that, but the link won't display properly if it's not in a hyperlink block: I'm not an expert with WiX or anything, so there may be another way. |
@joaocgreis absolutely, that's a good idea. We could pin |
Not very knowledgeable of Windows installer stuff so not going to LGTM this but +1 on having this! |
I would put some more emphasis on the fact that user need to install a the build tools. A lot of people do not read those sentences. To do this, I think we should also add a "UI" version of https://github.com/workshopper/goingnative/tree/master/exercises/am_i_ready to check everything is ok, and have it available also as a separate entry when the node installer starts. |
To be clear, this is amazing work. |
@mcollina I'm not sure if adding more messaging in the installer will help much. I think lots of people probably just mindlessly click next until it goes away. If people won't read a blurb after installation, another page or more detailed explanation may not help. Personally, I'm already 50/50 on this modification since it's deviating from the default WiX installer path, and making sure everything works is kind of challenging to test (see all the windows/installer tagged issues). |
@kaylynb I'm not sure either. This is a user experience problem, and tech should follow a proposed solution to fix it. IMHO, the process start "bad" from the download link on the main website: users download that blob and they expect it to contain everything they need. This is unfortunately not true. I'm 👍 on the instructions here. At least we are displaying a little warning. Another option is "block" installation if a compiler and python are not available (maybe with a "force" option, but...). |
If it were easier to include VS2015CE, and Python 2.x installers launched from the wix installer, that could be checkbox items, that may be best (unchecked by default) with a screen saying,
And only show if suitable tools aren't detected (per @mcollina 's comment/url above), and download/launch the online installers if checked. |
That would be the best course of action, if it is feasible.
|
we probably should be careful over-burdening on requirements here, perhaps work on the simplest thing first and move on from there? |
Redirect pointing to the wiki page, to be used by the Windows installer. Ref: nodejs/node#4111
Redirect pointing to the wiki page, to be used by the Windows installer. Ref: nodejs/node#4111
Redirect pointing to the wiki page, to be used by the Windows installer. Ref: nodejs/node#4111 PR-URL: nodejs#286 Reviewed-By: Alexis Campailla <[email protected]>
@kaylynb can you update the link to point to https://nodejs.org/windows-environment ? @fhemberger can you give us the German translation? It would be good to include it in the same commit. |
@joaocgreis that page returns a 404 currently, I would love to review the content. |
@mcollina sorry for not being clear. The server should be updated soon (ref nodejs/build#286), and the page will redirect to https://github.com/nodejs/node/wiki/Windows-Environment . I filled it with a simple initial version, straight to the point, and a link for the Node.js Guidelines repo. The idea is enabling people to make that page better, so feel free to review it! |
sorry for the hold-up, https://nodejs.org/windows-environment is active now! |
tools/msvs/msi/i18n/en-us.wxl
Outdated
@@ -35,4 +35,6 @@ | |||
|
|||
<!-- References like [ProductName] are not resolved for Property tags --> | |||
<String Id="WIXUI_EXITDIALOGOPTIONALTEXT">Node.js has been successfully installed.</String> | |||
|
|||
<String Id="InstalledNativeCompileInfo">Some native modules may require additional build tools to be installed. See the <![CDATA[<a href="https://github.com/Microsoft/nodejs-guidelines/blob/master/windows-environment.md#configuring-your-windows-development-environment">Windows Native Development Guide</a>]]> for more information.</String> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
German translation:
<String Id="InstalledNativeCompileInfo">Einige native Module benötigen eventuell zusätzliche Build-Tools um installiert werden zu können. Weitere Informationen finden Sie im <![CDATA[<a href="https://github.com/Microsoft/nodejs-guidelines/blob/master/windows-environment.md#configuring-your-windows-development-environment">Windows Native Development Guide</a>]]>.</String>
/cc @nodejs/nodejs-de
Added the new link destination to strings. Also the German translation. Since the new page doesn't have the same name as the Microsoft guide, I did reword the english a bit. Don't know if that's desired or if the German needs updating too. I won't be able to build the installer and try it out for a few days as I just setup my new workstation and don't have KVM setup yet. |
New text: Does anybody know how to get German to show up in the installer? I tried setting my system and user locales to de-DE, but the installer still used the English strings. I couldn't even get the official 5.4.1 msi to show localized strings. WiX seems to indicate that a seperate installer may be needed? |
@kaylynb Yes, just uncomment this section: https://github.com/kaylynb/node/blob/AddMSINativeModuleDescription/tools/msvs/msi/nodemsi.wixproj#L79 The German installer has been prepared but hasn't been enabled for builds so far, because at that time we had a release waiting to be pushed. |
Hey, just noticed this thread - awesome to see this work! Regarding the wiki page content, I've just submitted a PR to the node-gyp README (nodejs/node-gyp#867) that includes more complete install instructions (for both the VC++ Build Tools & VS2015 since they can't be installed side-by-side) that we may want to update the wiki page with. Thoughts? |
@mousetraps I think those instructions are great and highly needed, thanks :). |
@mousetraps thanks for working on that! One of us will review it properly as soon as possible. It might be good to try and unify the instructions somewhat, feel free to treat the wiki as a wiki and edit it where you think it needs it and perhaps update your PR to redirect people to there so we don't repeat ourselves. Great to have people focusing on Windows users btw, not enough of us are comfortable enough in Windows to look after the swathes of people who have trouble with this stuff. |
I did a more careful review (hoping to get this landed soon) but 2 problems popped up:
|
@joaocgreis what is still blocking this? |
@thealphanerd just the copyright notice. I'm not sure if it can simply be removed, or if we have to add our own, or what needs to be done. I don't know if there's anyone else that can be mentioned for this that's not already here. The |
7da4fd4
to
c7066fb
Compare
Is there any way to get the copyright question resolved? Or is this possibly in limbo indefinitely?
|
@mikeal @nodejs/tsc : do we have any resources at the Foundation for answering licensing questions? |
Given the inability of the Legal Committee to get back to the TSC on the questions we asked it many months ago I'd say the answer is no. I'm pretty sure that any staff of the NF or LF are not going to be willing to express an official opinion on anything for us without going through the Legal Committee and it's not really in a position to make quick enough progress on anything we need. |
@rvagg ... this is definitely a problem that needs to be raised and discussed at the next board meeting. A clear channel of communication with the legal committee on licensing issues is something the foundation must provide. |
c133999
to
83c7a88
Compare
Recommend closing given the lack of forward progress on this. |
In the interim, would it be possible to have a short url that redirects (ex: |
Feel free to reopen this PR if you get back to working on it. |
This is a concept for #3694.
It looks like the standard way to customize the final install dialog in WiX is to fork the built-in dialog from the WiX source and modify it with your custom fields (in this case, the Hyperlink control). This could add some maintenance headaches, so further discussion around messaging placement may be needed.
There are a few related issues as well: