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: Replace $(AT) with .SILENT #22283

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

dgoncharov
Copy link

This reduces the amount of syntax noise in the makefiles.
Setting V=1 still enables verbose logging.

The only noticeable difference in behavior is that, unless V=1 is specified, make won't print its own messages like
make: Nothing to be done for 'all', make: 'all' is up to date, or touch , if -t is specified.

@hebasto
Copy link
Member

hebasto commented Jun 19, 2021

s/.SILENCE/.SILENT/ in the PR name?

@hebasto
Copy link
Member

hebasto commented Jun 19, 2021

On master, for debugging purpose one could easy enable echoing for particular lines in a recipe by removing $(AT).

How to achieve the same goal with this PR?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 20, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22811 (build: Fix depends build system when working with subtargets by hebasto)
  • #22126 (build: Disable make builtin rules. by dgoncharov)
  • #21995 (build: Make built dependency packages reproducible by hebasto)

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.

@dgoncharov dgoncharov changed the title build: Replace $(AT) with .SILENCE. build: Replace $(AT) with .SILEN Jun 20, 2021
@dgoncharov dgoncharov changed the title build: Replace $(AT) with .SILEN build: Replace $(AT) with .SILENT Jun 20, 2021
@dgoncharov
Copy link
Author

s/.SILENCE/.SILENT/ in the PR name?

Indeed.

@dgoncharov
Copy link
Author

On master, for debugging purpose one could easy enable echoing for particular lines in a recipe by removing $(AT).

How to achieve the same goal with this PR?

You'd need to do the opposite. Which is to add @ in front of those lines which you want to silence. That'd be inconvenient.

You can do something similar though. If you are willing to modify the makefile, then you can add .SILENT: and make it depend on targets which you want to silence.

Some of these recipes run a sequence of shell commands bound together through && and ||. You won't be able to selectively enable or disable echoing parts of these either on master or on this branch. E.g. fetch_file_inner. These recipes would need to be written in a more conventional make style. Without && and ||, but rather on different recipe lines.

@dgoncharov
Copy link
Author

On master all the recipe lines have $(AT), on the branch none of them has.
This allows to come up with the opposite use case.

On the branch, for debugging purpose one could easy disable echoing for particular lines in a recipe by adding @.
You see, depending on how many lines you need to modify, could be easier on master or this branch.

@dongcarl
Copy link
Contributor

Can verify that .SILENT exists for both GNU Make and BSD Make (the only two UNIX implementations)

@dgoncharov
Copy link
Author

Can verify that .SILENT exists for both GNU Make and BSD Make (the only two UNIX implementations)

.SILENT exists in other make implementations as well as in posix. .SILENT was even supported by the very first make of Stuart Feldman.

@dgoncharov
Copy link
Author

i merged master onto this branch, resolved the coflict and pushed to my fork.
Is anything else needed in regards to the "needs rebase" label?

@fanquake
Copy link
Member

Is anything else needed in regards to the "needs rebase" label?

No, the label is removed by the bot, however you need to get rid of the merge commit here.

@maflcko
Copy link
Member

maflcko commented Jul 19, 2021

The following should do a proper rebase:

git reset --hard 04f41bbca0e462b70b9e5aa4115adf37db43735a
git rebase bitcoin-core/master

This reduces the amount of syntax noise in the makefiles.
@dgoncharov dgoncharov force-pushed the replace_AT_with_dotsilence branch from 5ee6f2e to 8494dca Compare July 19, 2021 12:14
@dgoncharov
Copy link
Author

Thanks, rebased.

@laanwj
Copy link
Member

laanwj commented Dec 17, 2021

Tested ACK 8494dca
Depends builds without and with V=1 work after this.

@laanwj laanwj merged commit 784a21d into bitcoin:master Dec 17, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 18, 2021
8494dca Replace $(AT) with .SILENCE. (Dmitry Goncharov)

Pull request description:

  This reduces the amount of syntax noise in the makefiles.
  Setting V=1 still enables verbose logging.

  The only noticeable difference in behavior is that, unless V=1 is specified, make won't print its own messages like
  make: Nothing to be done for 'all', make: 'all' is up to date, or touch <file>, if -t is specified.

ACKs for top commit:
  laanwj:
    Tested ACK 8494dca

Tree-SHA512: 66b9111229995aa54a9e87f4571648727d89b8529caec651063cdfe5c00a64341371b648701d192b2334df0614617a00c28eaa56c7f08ee9c00127cada0293ab
@bitcoin bitcoin locked and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants