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

build: use $(RM) in Makefile for consistency #12157

Merged
merged 1 commit into from
Apr 4, 2017
Merged

build: use $(RM) in Makefile for consistency #12157

merged 1 commit into from
Apr 4, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Apr 1, 2017 •
edited
Loading

Also allows someone to reassign $RM, e.g. with RM=rm -v instead of rm -f (the default) should they want to. We're currently using a mixture of $(RM) and rm -f.

The second commit changes the places where we weren't using the -f flag, I assume this wasn't on purpose, but if it was they should be left as is (if not I'll squash).

Note that we don't need the dash prefix (-rm), as -f already suppresses errors or output.

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

build

cc/ @nodejs/build @bnoordhuis @joyeecheung

EDIT: CI: https://ci.nodejs.org/job/node-test-commit/8826/ EDIT: CI was green

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 1, 2017
mhdawson
mhdawson approved these changes Apr 3, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@gibfahn
build: use $(RM) in Makefile for consistency
57b850e 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

PR-URL: nodejs#12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@gibfahn gibfahn merged commit 57b850e into nodejs:master Apr 4, 2017
@gibfahn gibfahn deleted the rm-makefile branch April 4, 2017 09:36
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@gibfahn @italoacasas
build: use $(RM) in Makefile for consistency
72a27b3 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

PR-URL: nodejs#12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

gibfahn added a commit to gibfahn/node that referenced this pull request Apr 19, 2017
@gibfahn
build: use $(RM) in Makefile for consistency
7abc0cc 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

PR-URL: nodejs#12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@gibfahn gibfahn mentioned this pull request Apr 19, 2017
Closed
4 tasks
@gibfahn
Copy link
Member Author

gibfahn commented Apr 19, 2017

Backport in #12515

MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@gibfahn @MylesBorins
build: use $(RM) in Makefile for consistency
a6e62b5 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

Backport-PR-URL: #12515
PR-URL: #12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@gibfahn @MylesBorins
build: use $(RM) in Makefile for consistency
e7e83f6 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

Backport-PR-URL: #12515
PR-URL: #12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@gibfahn @MylesBorins
build: use $(RM) in Makefile for consistency
6ddea5c 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

Backport-PR-URL: #12515
PR-URL: #12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@MylesBorins MylesBorins mentioned this pull request Apr 29, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@gibfahn @andrew749
build: use $(RM) in Makefile for consistency
0c5dfce 
Also allows someone to reassign `$RM`, e.g. with `RM=rm -v` instead of
`rm -f` (the default) should they want to. We're currently using a
mixture of `$(RM)` and `rm -f`.

There are a couple of places which aren't doing -f, have them do it for
consistency.

Backport-PR-URL: nodejs/node#12515
PR-URL: nodejs/node#12157
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@richardlau richardlau richardlau approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jbergstroem jbergstroem jbergstroem approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.