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: Fix build verbosity for V=0 #1066

Closed

Conversation

StefanJum
Copy link
Member

When setting V=0, we expect the verbosity level to be reduced, and
only the build output to be printed, not the build commands.
Do that by adding the -s option to MAKEFLAGS in case V is set to 0.

Signed-off-by: Stefan Jumarea stefanjumarea02@gmail.com
GitHub-Fixes: #676

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): [e.g. x86_64 or N/A]
  • Platform(s): [e.g. kvm, xen or N/A]
  • Application(s): [e.g. app-python3 or N/A]

Additional configuration

Description of changes

@StefanJum StefanJum requested a review from a team as a code owner August 22, 2023 17:45
@StefanJum StefanJum removed the request for review from a team August 22, 2023 17:46
@StefanJum StefanJum added kind/bug Something isn't working topic/build Topics to do with the build system bug/compile-time Bug occurs during compile-time bug/fix This PR fixes a bug size/small Small PR, quick to review labels Aug 22, 2023
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
ac0196e Makefile: Fix build verbosity for V=0

@unikraft-bot unikraft-bot added the area/makefile Part of the Unikraft Makefile build system label Aug 22, 2023
@razvand razvand requested review from kubanrob and Sairajkodilkar and removed request for adinasm September 8, 2023 21:19
@razvand razvand added this to the v0.15.0 (Pandora) milestone Sep 8, 2023
@kubanrob
Copy link
Contributor

kubanrob commented Sep 11, 2023

Hi @StefanJum!

Looking at the surrounding code, it seems the underlying problem is that if Q := @ only ever set if V was not set in the command line.

I suspect the code would be to understand / fixed if the following things were separated:

  • finding out what verbosity level we want (ifeq ("$(origin V)", "command line") ... else ... endif)
  • set the MAKEFLAGS / Q / KBUILD_VERBOSE according to the verbosity

Maybe this is a good chance to clean that up :)

Your solution works for me. However, if I'm not missing something, it seems a new solution to the problem Q is suppose to solve.

@skuenzer skuenzer self-assigned this Sep 14, 2023
@razvand razvand requested review from razvanvirtan and mariasfiraiala and removed request for kubanrob September 27, 2023 05:00
Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Hi @kubanrob, I don't really get your point. I've taken a look regarding the usage of Q and it doesn't look that related to what @StefanJum added.

Maybe I'm missing something though. Do correct me if I'm wrong. The changes work, so I'll add my tag after your request gets sorted out.

@razvand razvand requested a review from kubanrob October 1, 2023 01:44
Copy link
Contributor

@razvanvirtan razvanvirtan left a comment

Choose a reason for hiding this comment

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

Thanks, @StefanJum!
All good on my side.

Reviewed-by: Razvan Virtan virtanrazvan@gmail.com

@kubanrob
Copy link
Contributor

kubanrob commented Oct 4, 2023

Hello together (@mariasfiraiala), sorry for the late reply!

$Q is used as in the build command, setting it to @ makes the command not be printed by make:

So basically Q := @ mechanism aims to do the same as adding -s to the make flags. If you would set Q := @ for the condition were V=0 is set on the command line, the verbosity does not increase.

# To put more focus on warnings, be less verbose as default
# Use 'make V=1' to see the full commands
ifeq ("$(origin V)", "command line")
  BUILD_VERBOSE = $(V) 
else
  BUILD_VERBOSE = 0
endif

ifneq ($(BUILD_VERBOSE),0)
  KBUILD_VERBOSE := 1
  Q :=
else
  # Set KBUILD_VERBOSE and Q to quiet mode
  KBUILD_VERBOSE := 0
  Q := @
endif

So I do not see why -s needs to be added to the $MAKEFLAGS if this bug is fixed. Or if we add the -s, why bother with the $Q?
Even if both Q and the make-flag needs to be set for some reason, I think the condition for when it needs to be set should be reworked to set both consistently.

@StefanJum StefanJum force-pushed the StefanJum/fix-build-verbosity branch from ac0196e to 5f9b494 Compare October 4, 2023 13:20
@StefanJum
Copy link
Member Author

Indeed @kubanrob, looks like Q just was not set in case V=0. Please take another look now.

Makefile Outdated Show resolved Hide resolved
When setting `V=0`, we expect the verbosity level to be reduced, and
only the build output to be printed, not the build commands.
Set `Q = @` by default, in case `V != 0` is set, set `Q` appropriately.

Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>
GitHub-Fixes: unikraft#676
@StefanJum StefanJum force-pushed the StefanJum/fix-build-verbosity branch from 5f9b494 to d034171 Compare October 4, 2023 14:36
Copy link
Contributor

@kubanrob kubanrob left a comment

Choose a reason for hiding this comment

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

Thank you @StefanJum!

Reviewed-by: Robert Kuban robert.kuban@opensynergy.com

Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Works as expected, thanks!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand closed this in 2811157 Oct 4, 2023
razvand pushed a commit that referenced this pull request Oct 20, 2023
When setting `V=0`, we expect the verbosity level to be reduced, and
only the build output to be printed, not the build commands.
Set `Q = @` by default, in case `V != 0` is set, set `Q` appropriately.

Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>
GitHub-Fixes: #676
Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com>
Reviewed-by: Robert Kuban <robert.kuban@opensynergy.com>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: #1066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/makefile Part of the Unikraft Makefile build system bug/compile-time Bug occurs during compile-time bug/fix This PR fixes a bug kind/bug Something isn't working size/small Small PR, quick to review topic/build Topics to do with the build system
Projects
Status: Done!
Status: Done
Development

Successfully merging this pull request may close these issues.

Setting V=0 does not reduce verbosity
8 participants