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

makefile: clean test #322

Closed
wants to merge 1 commit into from
Closed

Conversation

zhouhao3
Copy link

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
@@ -35,7 +35,7 @@ clean:
$(RUNTIME_TOOLS_LINK):
ln -sf $(CURDIR) $(RUNTIME_TOOLS_LINK)

.PHONY: test .gofmt .govet .golint

Choose a reason for hiding this comment

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

Well, I think this is not a bug or wrong content.
Putting them into .PHONY is a way to defend in case of there is .gofmt, .govet and so son which will break make process.
As there is not any one in project directory, removing them or not is got a big deal.
But noting fix in comment is not suitable, cleanup will be better.

Copy link
Author

@zhouhao3 zhouhao3 Feb 10, 2017

Choose a reason for hiding this comment

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

Both cases can be implemented, and I did not find an accurate description of which is wrong. Just a personal feeling that would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting them into .PHONY is a way to defend in case of there is .gofmt, .govet and so son which will break make process.

And it's also a way of saying “these target names are just for convenience. We are not attempting to create files called .gofmt, etc.”. I'd rather leave them .PHONY so folks don't get confused (“Why is this recipe not creating a .gofmt file?”).

@zhouhao3 zhouhao3 changed the title makefile: fix test makefile: clean test Feb 10, 2017
@Mashimiao
Copy link

@mrunalp @hqhq @liangchenye How you think about it?

@mrunalp
Copy link
Contributor

mrunalp commented Feb 14, 2017

Let's leave those targets as PHONY.

@Mashimiao
Copy link

@q384566678 almost reviewers think we should keep it. So close it .

@Mashimiao Mashimiao closed this Feb 14, 2017
@zhouhao3 zhouhao3 deleted the makefile-test branch March 27, 2017 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants