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

Skip build if with installer & no WiX install found #5220

Closed
wants to merge 1 commit into from
Closed

Skip build if with installer & no WiX install found #5220

wants to merge 1 commit into from

Conversation

dimhotepus
Copy link
Contributor

build: Skip node build if with installer & no WiX

When building node on windows with installer, and supported now WiX
installations are not found (VS2013 / VS2015), skip build of node and
its installer.

@dimhotepus
Skip node build & notify user if WiX install is not found
74af255 
If none of VS2013/VS2015 WiX installs are found on a builder machine, skip node build and notify user about.
@mscdex mscdex added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Feb 13, 2016
@rvagg
Copy link
Member

rvagg commented Feb 15, 2016

lgtm, looks like an oversight, wix-not-found is not currently used /cc @nodejs/build

@dimhotepus would you mind reformatting your commit message to conform with the contributing guidelines @ https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit please?

@joaocgreis
Copy link
Member

LGTM

@dimhotepus dimhotepus changed the title Skip node build & notify user if WiX install is not found Skip build if with installer & no WiX install found Feb 15, 2016
@dimhotepus
Copy link
Contributor Author

I'm quite new to Git.
Tried to update comment, feel free to rewrite, if needed.

@jasnell
Copy link
Member

jasnell commented Feb 15, 2016

LGTM

rvagg pushed a commit that referenced this pull request Feb 16, 2016
@dimhotepus @rvagg
build: skip msi build if WiX is not found
f431984 
If none of VS2013/VS2015 WiX installs are found on a builder
machine, skip node build and notify user about.

PR-URL: #5220
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Feb 16, 2016

No problems @dimhotepus, feel free to ask for help when you need it, we were all git newbies once and some of us feel like we are still

In this case, you needed to git commit --amend and you would get the commit message file again where you can change the message to conform to the requirements here. The three things you were missing were (1) a subsystem, in this case a build: prefix, (2) 50 char limit of commit summary and (3) line wrapping of the description at 72 columns. I know it seems onerous but you get used to it after a while and it does help us keep records in order.

I've landed this @ f431984, feel free to go there and have a look how I structured it. Thanks for the PR!

@rvagg rvagg closed this Feb 16, 2016
rvagg pushed a commit that referenced this pull request Feb 18, 2016
@dimhotepus @rvagg
build: skip msi build if WiX is not found
fc78d3d 
If none of VS2013/VS2015 WiX installs are found on a builder
machine, skip node build and notify user about.

PR-URL: #5220
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
@dimhotepus @stefanmb
build: skip msi build if WiX is not found
2fb285e 
If none of VS2013/VS2015 WiX installs are found on a builder
machine, skip node build and notify user about.

PR-URL: nodejs#5220
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2016
Closed
@MylesBorins
Copy link
Contributor

@rvagg do you think this should be added to LTS?

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

Likely safe. @nodejs/lts

@joaocgreis
Copy link
Member

+1 for adding to LTS, this was an oversight and should be corrected.

@rvagg
Copy link
Member

rvagg commented Mar 14, 2016

+1

MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
@dimhotepus
build: skip msi build if WiX is not found
47e8c5c 
If none of VS2013/VS2015 WiX installs are found on a builder
machine, skip node build and notify user about.

PR-URL: #5220
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@dimhotepus
build: skip msi build if WiX is not found
46a5d51 
If none of VS2013/VS2015 WiX installs are found on a builder
machine, skip node build and notify user about.

PR-URL: #5220
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
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. 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
@dimhotepus @rvagg @joaocgreis @jasnell @MylesBorins @mscdex