FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

win,msi: fix inclusion of translations by joaocgreis · Pull Request #7798 · nodejs/node · GitHub

/ node Public

win,msi: fix inclusion of translations - #7798

Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom
JaneaSystems:joaocgreis-G7J-fix-msi
ConversationCommits (1)ChecksFiles changed
Closed

win,msi: fix inclusion of translations#7798
joaocgreis wants to merge 1 commit into
nodejs:masterfrom
JaneaSystems:joaocgreis-G7J-fix-msi

Conversation

joaocgreis commented Jul 19, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

Build, MSI.

Description of change

This PR fixes two issues with the MSI:

cc @nodejs/build @nodejs/platform-windows @pmq20 @mcollina

joaocgreis added windows Issues and PRs related to the Windows platform. install Issues and PRs related to the installers. build Issues and PRs related to build files or the CI. labels Jul 19, 2016
joaocgreis mentioned this pull request Jul 19, 2016
Merged
mscdex changed the title win,,msi: fix inclusion of translations win,msi: fix inclusion of translations Jul 19, 2016
evanlucas reviewed Jul 19, 2016
View reviewed changes
Comment thread tools/msvs/msi/nodemsi.wixproj
<ItemGroup>
<!-- <EmbeddedResource Include="i18n\de-de.wxl" /> -->
<EmbeddedResource Include="i18n\en-us.wxl" />
<!--

evanlucas Jul 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

does this not comment out all but english?

mcollina Jul 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what's the plan on releasing with the translations?

joaocgreis Jul 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@evanlucas yes, this is supposed to be commented for now. The translations are not yet active, the installer only has english.

@fhemberger is there a plan to release with translations? Are we just waiting for more translations to arrive? On the build side, we use the stable release of WiX, so english and german are the only languages supported. The WiX website recommends not to use the weekly versions in production, but we could perhaps add just the translation files to the release version, if it works.

piccoloaiutante Jul 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@joaocgreis the issue there is that WiX guys are going to make a new release only for the next Visual Studio release so it could take time

wixtoolset/wix3#366 (comment)

piccoloaiutante Jul 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@joaocgreis @fhemberger is using weekly build of WiX a viable option for building node installer?

joaocgreis Jul 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@piccoloaiutante I'm a little hesitant to use a weekly build because the WiX website says not to, and breaking the MSI is something that would cause major pain for users. But I'd be willing to give it a try, provided that we build release candidates for a few versions and get them well tested. Using the translations with the release version still sounds better.

Do we have a plan for when to start shipping with this?

piccoloaiutante Jul 25, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@joaocgreis i don't have any visibility on when shipping localized installer

mcollina commented Jul 20, 2016

Copy link
Copy Markdown
Member

I have a very little experience in all of this.. cc @piccoloaiutante might help.

evanlucas commented Jul 20, 2016

Copy link
Copy Markdown
Contributor

If those translations are not even supported, I think I would prefer to back them out of the 6.3.1 proposal, at least until we figure this out. Thanks!

joaocgreis commented Jul 20, 2016

Copy link
Copy Markdown
Member Author

@evanlucas sounds good, for end users it's the same as it is now.

joaocgreis commented Jul 22, 2016

Copy link
Copy Markdown
Member Author

@nodejs/platform-windows can you please review? This will fix the nightlies.

joaocgreis commented Jul 27, 2016

Copy link
Copy Markdown
Member Author

@pmq20 @dead-horse @yorkie @fhemberger @XadillaX @mcollina @rvagg you submitted and reviewed the original PRs, this should be an easy review for you. Can you please take a look to get the releases unbroken and the nightlies flowing again?

bzoz commented Aug 1, 2016

Copy link
Copy Markdown
Contributor

LGTM

joaocgreis added a commit that referenced this pull request Aug 3, 2016
win,msi: fix inclusion of translations
c9e670e 
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
PR-URL: #7798

joaocgreis commented Aug 3, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/3478/ (freebsd failure unrelated)

Landed in c9e670e

joaocgreis closed this Aug 3, 2016
joaocgreis mentioned this pull request Aug 3, 2016
Closed
cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
win,msi: fix inclusion of translations
f1c50a8 
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
PR-URL: #7798
cjihrig mentioned this pull request Aug 11, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
win,msi: fix inclusion of translations
b3634ad 
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
PR-URL: #7798
rvagg pushed a commit that referenced this pull request Oct 18, 2016
win,msi: fix inclusion of translations
ddeed99 
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
PR-URL: #7798
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
win,msi: fix inclusion of translations
1b5213b 
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
PR-URL: #7798
MylesBorins mentioned this pull request Oct 26, 2016
Closed
bnoordhuis mentioned this pull request Oct 23, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

build Issues and PRs related to build files or the CI. install Issues and PRs related to the installers. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL