Skip to content

Conversation

@jboarman
Copy link
Contributor

@jboarmanjboarman commented Dec 4, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Corrected parameter for running tests on Windows. Without the corrected
parameters, Windows users encounter an error about failing to sign the
build, "Failed to sign exe", which can be discouraging to new Windows
community members.

Corrected parameter for running tests on Windows. Without the corrected parameters, Windows users encounter an error about failing to sign the build, "Failed to sign exe", which can be discouraging to new Windows community members.
@nodejs-github-botnodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 4, 2016
@addaleaxaddaleax added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Dec 4, 2016
@Trott
Copy link
Member

Trott commented Dec 5, 2016

@nodejs/platform-windows

Copy link
Contributor

@seishunseishun left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd prefer vcbuild nosign test.

bzoz
bzoz approved these changes Dec 5, 2016
@gibfahn
Copy link
Member

gibfahn commented Dec 9, 2016

I think this would be made unnecessary by #10156, which will also stop people having to remember the nosign argument.

@jasnell
Copy link
Member

#10156 landed. Is this still necessary?

@gibfahn
Copy link
Member

gibfahn commented Dec 23, 2016

@jasnell#10156 was semver-major, so I guess it still makes sense to do this for v[7,6,4]?

+1 for vcbuild test nosign->vcbuild nosign test

I'd also prefer changing .\vcbuild -> vcbuild for consistency (also on the build line above).

EDIT: we should be using .\vcbuild as per @richardlau's comment

cc/ @joaocgreis

@richardlau
Copy link
Member

@gibfahn.\vcbuild works on both cmd.exe and PowerShell on Windows (vcbuild does not work on PowerShell, see #8704).

Copy link
Member

@joaocgreisjoaocgreis left a comment

Choose a reason for hiding this comment

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

LGTM for v[4,6,7].

@jasnell
Copy link
Member

If this is a backport for 7, 6 and 4, then, separate PRs for landing in those should be opened.

@jboarman
Copy link
ContributorAuthor

I'm thinking that #10156 now makes this PR un-necessary.

@jboarmanjboarman closed this Jan 6, 2017
@jboarmanjboarman deleted the my-branch branch January 6, 2017 18:44
@gibfahn
Copy link
Member

gibfahn commented Jan 6, 2017

@jboarman see #10112 (comment), it's necessary for all current release lines as that PR is semver-major.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.docIssues and PRs related to the documentations.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@jboarman@Trott@gibfahn@jasnell@richardlau@joaocgreis@seishun@bzoz@addaleax@nodejs-github-bot