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

make defaults to quiet output #1098

Closed
wants to merge 1 commit into from
Closed

Conversation

Rangi42
Copy link
Member

@Rangi42 Rangi42 commented Dec 18, 2023

This may help beginners who don't notice success/error messages among all the make noise. Experienced users can run make Q=, or modify their Makefile to do so by default.

$ make clean
Deleting build products...
Deleting intermediate build products...

$ make compare -j
Building tools...
Checking RGBDS version...
Finding VC patch values...
Building ROM pokecrystal_debug.gbc...
Building ROM pokecrystal11_debug.gbc...
Building ROM pokecrystal.gbc...
Building ROM pokecrystal11.gbc...
Building ROM pokecrystal11_vc.gbc...
Building ROM pokecrystal_au.gbc...
Building VC patch pokecrystal11.patch...
Comparing output ROMs with originals...
pokecrystal.gbc: OK
pokecrystal11.gbc: OK
pokecrystal_au.gbc: OK
pokecrystal_debug.gbc: OK
pokecrystal11_debug.gbc: OK
pokecrystal11.patch: OK

$ make -j
Building tools...
make: Nothing to be done for 'all'.

The downside is that seeing all those individual commands is a definite indicator that progress is happening; but I think just waiting quietly is also okay, since the process isn't going to infinitely loop/hang, there'd be a visible error message. (Also, even currently, there's a rather long wait period before any messages show up, I think because scan_includes is running.)

I'm not committed to this change, just wanted feedback on a concrete implementation. Like if those explicit echoed messages should be edited or have more/fewer of them.

@Rangi42
Copy link
Member Author

Rangi42 commented Dec 18, 2023

(Even if we don't want this by default, I'd like to leave all the Qs in so you can explicitly do make Q=@.)

@Rangi42
Copy link
Member Author

Rangi42 commented Dec 18, 2023

(As for when there are error messages from users' modifications, @ISSOtm 's rsgbds 🦀 rewrite will be making those more readable too. ❤️ )

@Rangi42
Copy link
Member Author

Rangi42 commented Dec 18, 2023

Question: Should CI have "un-quiet" output? Again, if/when there is an error, we'd see it regardless, so I don't know if seeing the individual commands is useful or just noise to scroll through in GitHub's lazy-loading log viewer.

@dannye
Copy link
Member

dannye commented Dec 18, 2023

Personally, I'm not a fan. I'm fine with the structure being put in place, but I don't want it quiet by default.

@Rangi42
Copy link
Member Author

Rangi42 commented Dec 18, 2023

Okay. In that case, we don't even need the $(Q)s since make supports -s/--silent for opt-in quiet. Just the @echos so even when silent, you'll see some progress. (I would still like other devs' feedback here.)

@ISSOtm
Copy link
Contributor

ISSOtm commented Dec 18, 2023

Two middle grounds: µcity echoes merely rgbasm file.asm (downside: that looks like a command but it's not), and kernel projects instead display shorthands like ASM file.asm (downside: it's not very clear what those are).

Also, manually outputting commands in that way wouldn't honor -s unless scanning MAKEFLAGS.

@mid-kid
Copy link
Member

mid-kid commented Dec 18, 2023

Not a fan in general, especially for a project that builds from the top-level and has little command line noise like this one. Debugging any sort of build system issue will always involve re-running the entire thing, which might not trigger the same issue again.

That said, I'd consider something like this for out-of-tree builds with a more bespoke makefile, as the paths wouldn't make sense anymore.

@Rangi42
Copy link
Member Author

Rangi42 commented Dec 18, 2023

Alright, thanks for the feedback!

@Rangi42 Rangi42 closed this Dec 18, 2023
@Rangi42 Rangi42 deleted the make-quiet branch December 18, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants