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: avoid passing kill empty input in Makefile #12158

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Apr 1, 2017

Using xargs -r on some platforms and xargs on others doesn't work,
we can't guarantee whether xargs is GNU or not. Avoid the issue by only
running kill if there are processes to clean.

I ran into this as I'm on macOS, but have a GNU xargs (from brew install findutils).

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

build

cc/ @Trott @nodejs/build

EDIT: CI: https://ci.nodejs.org/job/node-test-commit/8825/ - CI is green

@gibfahn gibfahn requested a review from Trott April 1, 2017 18:59
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 1, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Much better. Thanks for this. LGTM if CI is green.

Using `xargs -r` on some platforms and `xargs` on others doesn't work,
we can't guarantee whether xargs is GNU or not. Avoid the issue by only
running kill if there are processes to clean.

PR-URL: nodejs#12158
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn merged commit d19809a into nodejs:master Apr 4, 2017
@gibfahn gibfahn deleted the xargs-makefile branch April 4, 2017 09:49
@jasnell jasnell mentioned this pull request Apr 4, 2017
@italoacasas
Copy link
Contributor

cc @gibfahn

@gibfahn gibfahn restored the xargs-makefile branch April 11, 2017 09:49
@gibfahn gibfahn deleted the xargs-makefile branch April 11, 2017 09:49
@gibfahn
Copy link
Member Author

gibfahn commented Apr 11, 2017

v7.x backport: #12337

gibfahn added a commit to gibfahn/node that referenced this pull request May 22, 2017
Using `xargs -r` on some platforms and `xargs` on others doesn't work,
we can't guarantee whether xargs is GNU or not. Avoid the issue by only
running kill if there are processes to clean.

PR-URL: nodejs#12158
@gibfahn
Copy link
Member Author

gibfahn commented Jun 17, 2017

Should land after #11246

gibfahn added a commit that referenced this pull request Jun 18, 2017
Using `xargs -r` on some platforms and `xargs` on others doesn't work,
we can't guarantee whether xargs is GNU or not. Avoid the issue by only
running kill if there are processes to clean.

PR-URL: #12158
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn added a commit that referenced this pull request Jun 20, 2017
Using `xargs -r` on some platforms and `xargs` on others doesn't work,
we can't guarantee whether xargs is GNU or not. Avoid the issue by only
running kill if there are processes to clean.

PR-URL: #12158
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Using `xargs -r` on some platforms and `xargs` on others doesn't work,
we can't guarantee whether xargs is GNU or not. Avoid the issue by only
running kill if there are processes to clean.

PR-URL: #12158
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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.

7 participants