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: Mark print-% target as phony. #22234

Merged
merged 1 commit into from
Jul 18, 2021
Merged

build: Mark print-% target as phony. #22234

merged 1 commit into from
Jul 18, 2021

Conversation

dgoncharov
Copy link

.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on force and mark force as phony.

This change ensures print-% rule works even when there is a file that matches the target.

$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch force
$ make print-host
host=x86_64-pc-linux-gnu

@hebasto
Copy link
Member

hebasto commented Jun 13, 2021

Also Makefile.am and src/Makefile.am?

@dgoncharov
Copy link
Author

Sure.

@DrahtBot DrahtBot mentioned this pull request Jun 13, 2021
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 13, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@dongcarl
Copy link
Contributor

Nice! Wondering why there's a difference in how you do it for Makefiles and Makefile.ams

@dgoncharov
Copy link
Author

What is the difference?

@dongcarl
Copy link
Contributor

What is the difference?

  1. It seems like you capitalize FORCE in Makefile.am and src/Makefile.am, whereas it is force in depends/Makefile
  2. You do .PHONY: force in depends/Makfile, but not in Makefile.am or src/Makefile.am

@dgoncharov
Copy link
Author

Both of these differences are accidental.
i initially planned to only modify depends/Makefile. So, i added target 'force'.
Then Hennadii pointed out that Makefile.am and src/Makefile.am have the same print-% rule.
So, i modified Makefile.am and src/Makefile.am. Both Makefile.am and src/Makefile.am already had a phony target called 'FORCE'. This eliminated the need for 'force' and i made print-% depend on 'FORCE' in Makefile.am and src/Makefile.am.

@dongcarl
Copy link
Contributor

Ah right! Let's just capitalize the one in depends/Makefile to make them consistent.

Code Review ACK a27d4a3 modulo FORCE capitalization in depends/Makefile

@DrahtBot DrahtBot mentioned this pull request Jun 20, 2021
Merged
@dongcarl
Copy link
Contributor

Code Review ACK 020802c

We might need to squash the commits before merge, but everything else looks good

@fanquake
Copy link
Member

Yes this needs squashing.

@gtkiller
Mark print-% target as phony.
fb7be92 
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on FORCE and mark FORCE as phony.

$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch FORCE
$ make print-host
host=x86_64-pc-linux-gnu
hebasto
hebasto approved these changes Jul 17, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fb7be92, tested on Linux Mint 20.2 (x86_64).

@fanquake fanquake merged commit b588961 into bitcoin:master Jul 18, 2021
@dgoncharov dgoncharov deleted the mark_print_as_phony branch July 18, 2021 19:54
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021
@fanquake
Merge bitcoin#22234: build: Mark print-% target as phony.
f84bba7
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge b588961 into merged_master (Bitcoin PR bitcoin/bitcoin#22234)
6bd2a81
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dgoncharov @hebasto @DrahtBot @dongcarl @fanquake @gtkiller