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.

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

doc: corrected vcbuild parameters for testing on windows #10112

Closed
wants to merge 1 commit into from

Conversation

jboarman
Copy link
Contributor

@jboarman jboarman 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-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 4, 2016
@addaleax addaleax 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

@seishun seishun 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.

@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

jasnell commented Dec 23, 2016

#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

@joaocgreis joaocgreis 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

jasnell commented Jan 6, 2017

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

@jboarman
Copy link
Contributor Author

jboarman commented Jan 6, 2017

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

@jboarman jboarman closed this Jan 6, 2017
@jboarman jboarman 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants