-
Notifications
You must be signed in to change notification settings - Fork 328
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 enhancements #1172
Make enhancements #1172
Conversation
Hi, |
On Wed, 06 Mar 2024 01:21:06 -0800 hongzhidao ***@***.***> wrote:
Hi,
I'm used to the current way, and it's working well for me.
You can still get the verbose output with `make V=1 ...`
I can't say whether this change is better or not, but I'd suggest inviting others to review this PR, thanks.
Cc: @callahad
This is all part of the overarching plan to present Unit as a modern C
project.
|
Rebased with master
|
Is there a precedent for using the D/E/V vars in that way? I haven't personally run across that before. |
Well apart from unit-wasm, the Linux Kernel has these
Git uses There is also a
|
So a few others indeed... |
That's compelling. Ultimately, this is a developer-facing subjective / aesthetic change... I'd be inclined for us to default to accepting these kinds of changes so long as there is not a strong objection from within the team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks reasonable to me, and CI shows it still builds correctly.
Remove an erroneous blank line.
|
I suggest adding an empty variable That way, it's generic, and you allow setting any extra CFLAGS at make(1) time. That's what I do in the Linux man-pages (I was going to suggest you have a look at it, but now I remember, that Makefile is GPLd :). I usually do |
Just FYI, GNU Make 4.4 (I think it was 4.4, don't remember the exact version) has added |
Interesting, but yes, 3.x is still prevalent. Also because we still print the new output in verbose mode you can do
on other makes to get the same effect. |
|
Lots of small improvemnets as suggested by @alejandro-colomar
|
The idea is rather than printing out the full compiler/linker etc command for each recipe e.g cc -c -pipe -fPIC -fvisibility=hidden -O0 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wno-strict-aliasing -Wmissing-prototypes -g -I src -I build/include \ \ \ -o build/src/nxt_cgroup.o \ -MMD -MF build/src/nxt_cgroup.dep -MT build/src/nxt_cgroup.o \ src/nxt_cgroup.c Print a clearer abbreviated message e.g the above becomes CC build/src/nxt_cgroup.o This vastly reduces the noise when compiling and most of the time you don't need to see the full command being executed. This also means that warnings etc show up much more clearly. You can still get the old verbose output by passing V=1 to make e.g $ make V=1 ... NOTE: With recent versions of make(1) you can get this same, verbose, behaviour by using the --debug=print option. This introduces the following message types CC Compiling a source file to an object file. AR Producing a static library, .a archive file. LD Producing a dynamic library, .so DSO, or executable. VER Writing version information. SED Running sed(1). All in all this improves the developer experience. Subsequent commits will make use of this in the core and modules. NOTE: This requires GNU make for which we check. On OpenIndiana/illumos we have to use gmake(1) (GNU make) anyway as the illumos make doesn't work with our Makefile as it is. Also macOS seems to generally install GNU make. We could make it work (probably) on other variants of make, but the complexity starts increasing exponentially. In fact we still print the abbreviated messages in the verbose output so you can still do $ make | grep ^" [A-Z]" on other makes to effectively get the same output. Co-developed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in the previous commit to pretty print the make output when building the Unit core and the C test programs. When building Unit the output now looks like VER build/include/nxt_version.h (NXT_VERSION) VER build/include/nxt_version.h (NXT_VERNUM) CC build/src/nxt_lib.o CC build/src/nxt_gmtime.o ... CC build/src/nxt_cgroup.o AR build/lib/libnxt.a CC build/src/nxt_main.o LD build/sbin/unitd SED build/share/man/man8/unitd.8 I'm sure you'll agree that looks much nicer! You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to pretty print the make output when building the Java language module. You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to pretty print the make output when building the Perl language module. You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to pretty print the make output when building the PHP language module. You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to pretty print the make output when building the Python language module. You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to pretty print the make output when building the Ruby language module. You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to pretty print the make output when building the wasm language module. You can still get the old verbose output with $ make V=1 ... Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
One issue you have when trying to debug Unit under say GDB is that at the default optimisation level we use of -O (-O1) the compiler will often optimise things out which means they are not available for inspection in the debugger. This patch allows you to pass 'D=1' to make, e.g $ make D=1 ... Which will set -O0 overriding the previously set -O, basically disabling optimisations, we could use -Og, but the clang(1) man page says this is best and it seems to not cause any issues when debugging GCC generated code. Co-developed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Having -Werror enabled all the time when developing can be a nuisance, allow to disable it by passing E=0 to make, e.g $ make E=0 ... This will set -Wno-error overriding the previously set -Werror. Co-developed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This adds a help target to the Makefile in the repository root that shows what variables are available to control the make/build behaviour. It currently looks like $ make help Variables to control make/build behaviour: make V=1 ... - Enables verbose output make D=1 ... - Enables debug builds (-O0) make E=0 ... - Disables -Werror Variables can be combined. Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This variable is _appended_ to the main CFLAGS variable and allows setting extra compiler options at make time. E.g $ make EXTRA_CFLAGS="..." ... Useful for quickly testing various extra warning flags. Suggested-by: Alejandro Colomar <alx@kernel.org> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Rebase with master
|
This PR comprises three main elements centred around improving the developer experience
The first item changes (by default) the make output from
to
It shows clearly what is happening and what the output is. It vastly reduces the noise and makes things like warnings stand out much more clearly. (See the first commit message for an explanation of these types).
This style of make output is used by projects such as the Linux Kernel and git.
I'm sure you'll agree it looks much nicer and improves the developer experience.
You can still get the verbose output by passing
V=1
to make, e.gThe next item is to enable debug builds. By this I mean builds that debuggable under the likes of GDB. We default to building with -O (-O1). This often has the issue of when debugging that things get optimised out and you can't inspect them.
With this change you can pass
D=1
to make to have it build with -O0, no optimisations.The third item allows you to disable -Werror at make time. Often having the build abort on what would normally just be a compiler warning gets in the way of development. You often make some quick change to test something, knowing you will get a warning only to have the build totally fail...
To remove this obstacle you can now pass
E=0
to make which will not enable-Werror
.For me I would often be using the following
It is worth noting that this new functionality does require GNU make. That covers Linux & macOS (by the looks of it). On OpenIndiana/illumos, you have to use gmake (GNU make) anyway.
Everything still works as before on non GNU make.
It would perhaps not be unreasonable to mandate GNU make, on FreeBSD for example gmake is readily installable. The likes of git for example requires GNU make... but that's perhaps a discussion for another day.