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: there are two bench phony targets in the makefile #16968

Closed
joyeecheung opened this issue Nov 12, 2017 · 7 comments
Closed

build: there are two bench phony targets in the makefile #16968

joyeecheung opened this issue Nov 12, 2017 · 7 comments
Labels
build Issues and PRs related to build files or the CI. good first issue Issues that are suitable for first-time contributors.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Nov 12, 2017 •
edited
Loading

  • Version: master
  • Subsystem: build

What the title says. One of them should be removed.

node/Makefile

Lines 1121 to 1122 in c5a49e1

bench \
bench \

@joyeecheung joyeecheung added build Issues and PRs related to build files or the CI. good first issue Issues that are suitable for first-time contributors. labels Nov 12, 2017
@ivancsicsmarkus ivancsicsmarkus mentioned this issue Nov 12, 2017
Closed
@addaleax addaleax removed the good first issue Issues that are suitable for first-time contributors. label Nov 13, 2017
@bnoordhuis
Copy link
Member

Not really a bug. Multiple .PHONY targets are allowed and even encouraged by some style guides.

Since we're discussing this, maybe it's a good idea to switch to this style:

.PHONY: foo
foo:
        make_foo

.PHONY: bar
bar:
        make_bar

Makes it more obvious it's phony vis-a-vis hiding that fact at the end of the file.

@targos targos added the good first issue Issues that are suitable for first-time contributors. label Jan 2, 2018
@targos
Copy link
Member

targos commented Jan 2, 2018

Added good first issue label for Ben's suggestion.

@oantoro
Copy link
Contributor

oantoro commented Jan 3, 2018

Hello @targos, @bnoordhuis
Should we make it like:

.PHONY: foo
foo:
        make_foo

.PHONY: bar
bar:
        make_bar

# Redefinition
.PHONY: foo \
  bar

Maybe I can work on this

@bnoordhuis
Copy link
Member

@okyantoro Yes, but not the block you labeled 'redefinition.'

@oantoro
Copy link
Contributor

oantoro commented Jan 3, 2018

@bnoordhuis I am still working on this. But I find targets below have no matching target inside Makefile.

bench-http-simple \
  bench-idle \
  blog \
  blogclean \
  dist \
  dynamiclib \
  install-bin \
  install-includes \
  staticlib \
  website-upload

Is it okay to delete these targets?

@bnoordhuis
Copy link
Member

@okyantoro Yes, that's fine.

oantoro added a commit to oantoro/node that referenced this issue Jan 3, 2018
@oantoro
build: put .PHONY on line before of its target definition.
752cbec 
Before this change, the .PHONY is followed by multiple targets. Now it is multiple .PHONY for each target

Refs: nodejs#16968
@oantoro oantoro mentioned this issue Jan 3, 2018
Closed
2 tasks
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 5, 2018
@oantoro @BridgeAR
build: put .PHONY directly before its target
feaf6ac 
Before this change, the .PHONY is followed by multiple targets.
Now it is multiple .PHONY for each target.

PR-URL: nodejs#17964
Refs: nodejs#16968
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos
Copy link
Member

targos commented Jan 7, 2018

Fixed in #17964
Thank you @okyantoro !

@targos targos closed this as completed Jan 7, 2018
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
@oantoro @MylesBorins
build: put .PHONY directly before its target
64e6e46 
Before this change, the .PHONY is followed by multiple targets.
Now it is multiple .PHONY for each target.

PR-URL: #17964
Refs: #16968
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
@oantoro @MylesBorins
build: put .PHONY directly before its target
84883f2 
Before this change, the .PHONY is followed by multiple targets.
Now it is multiple .PHONY for each target.

PR-URL: #17964
Refs: #16968
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
@oantoro @MylesBorins
build: put .PHONY directly before its target
224033d 
Before this change, the .PHONY is followed by multiple targets.
Now it is multiple .PHONY for each target.

PR-URL: #17964
Refs: #16968
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@bnoordhuis @addaleax @targos @joyeecheung @oantoro