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

build: correct minor typo in lttng help message #16101

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Oct 9, 2017

Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 9, 2017
@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2017

If this is for consistency with

node/configure

Line 888 in 84579b1

raise Exception('lttng is only supported on Linux.')

why not change that line so it matches?

-raise Exception('lttng is only supported on Linux.') 
+raise Exception('Lttng is only available on Linux.') 

@danbev
Copy link
Contributor Author

danbev commented Oct 10, 2017

If this is for consistency with

I was mainly thinking of the only supported to Linux which didn't sound right to me. I think the rest of the message is consistent with other flags like --with-etw.

@danbev
Copy link
Contributor Author

danbev commented Oct 11, 2017

@gibfahn Just wanted to double check this with you before I land it in regards to the comment above.

@gibfahn
Copy link
Member

gibfahn commented Oct 11, 2017

it just seems weird for the help to say:

build with Lttng (Only available on Linux)')

and the error to say:

lttng is only supported on Linux.

Why not make them both available or both supported?

@danbev
Copy link
Contributor Author

danbev commented Oct 11, 2017

Why not make them both available or both supported?

Ah I see now. I've updated it to supported. Thanks

danbev added a commit to danbev/node that referenced this pull request Oct 12, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: nodejs#16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@danbev
Copy link
Contributor Author

danbev commented Oct 12, 2017

Landed in 6cc4cf7

@danbev danbev closed this Oct 12, 2017
@danbev danbev deleted the configure-lttng-typo branch October 12, 2017 09:04
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: nodejs/node#16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: #16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: #16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: #16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: #16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Currently the help message when using --with-lttng looks like this:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available to Linux)

This commit makes the help message consistent with the error message
that is raised if --with-lttng is used on a non-Linux operating
system:

$ ./configure --help | grep 'with-lttng'
  --with-lttng          build with Lttng (Only available on Linux)

PR-URL: #16101
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants