xmake-io / xmake

🔥 A cross-platform build utility based on Lua
https://xmake.io
Apache License 2.0
10.04k stars 786 forks source link

Flag `-w, --warning` does not work. #2452

Closed szanni closed 2 years ago

szanni commented 2 years ago

Xmake Version

xmake v2.6.7+master.ad3d6a565

Operating System Version and Architecture

Linux t480 5.17.4-arch1-1 #1 SMP PREEMPT Wed, 20 Apr 2022 18:29:28 +0000 x86_64 GNU/Linux

Describe Bug

When I run xmake -w or xmake --warning no warnings get printed.

The documentation however says: -w, --warning Enable the warnings output.

xmake seems to silence all stderr output.

Expected Behavior

I would expect compile time warnings to be printed to the command line.

I actually expect compile time warnings to ALWAYS get printed when setting set_warnings() [exception of course set_warnings("none")].

This is how all other build tools I know handle this.

Project Configuration

xmake create -l c -t console test
cd test

Edit xmake.lua:

add_rules("mode.debug", "mode.release")

set_languages("c99")
set_warnings("allextra")

target("test")
    set_kind("binary")
    add_files("src/*.c")

Run xmake -w.

Additional Information and Error Logs

Even when I run xmake -wv I do not see any warnings printed.

When I run the compile commands manually printed by xmake -v, I do get warnings printed: /usr/bin/gcc -c -m64 -fvisibility=hidden -Wall -Wextra -O3 -std=c99 -DNDEBUG -o build/.objs/test/linux/x86_64/release/src/main.c.o src/main.c

src/main.c: In function ‘main’:
src/main.c:3:14: warning: unused parameter ‘argc’ [-Wunused-parameter]
    3 | int main(int argc, char** argv)
      |          ~~~~^~~~
src/main.c:3:27: warning: unused parameter ‘argv’ [-Wunused-parameter]
    3 | int main(int argc, char** argv)
      |   
waruqi commented 2 years ago

This is due to the new version's built-in ccache, which you can disable to get the warning output. xmake f --ccache=n

I've just updated the dev version with some fixes that you can also try. xmake update -s dev

However, the warnings are currently only output for the first build, as the second build uses the cache directly, so the warnings are still not displayed. I can't think of a better way to do this yet, unless the warning output is cached as well.

szanni commented 2 years ago

This is due to the new version's built-in ccache, which you can disable to get the warning output. xmake f --ccache=n

Disabling ccache does work. This must be an issue with the lua wrapper though? Because running ccache directly does print warnings.

I've just updated the dev version with some fixes that you can also try. xmake update -s dev

Yes, definitely does print warnings now. Thanks for the quick response.

However, the warnings are currently only output for the first build, as the second build uses the cache directly, so the warnings are still not displayed. I can't think of a better way to do this yet, unless the warning output is cached as well.

I am not quite sure if I understand. Compilers only print warnings when a file is compiled. If the file has not changed and therefore is not compiled: no warning printed.

What is still broken is xmake -wr. It should print warnings when rebuilding.

The other problem I have is needing to runxmake -w. I expect set_warnings() to automatically enable that. This is what meson does.

There should be an option to silence warnings instead in my opinion.

Should I open a separate issue for that?

waruqi commented 2 years ago

Disabling ccache does work. This must be an issue with the lua wrapper though? Because running ccache directly does print warnings.

xmake v2.6.7 use builtin build cache instead of 3rd ccache command.

Compilers only print warnings when a file is compiled. If the file has not changed and therefore is not compiled: no warning printed.

What is still broken is xmake -wr. It should print warnings when rebuilding.

xmake/ccache will cache build object files to speed up rebuilding.

So when you re-build everything, xmake/ccache will also use the cached object files directly.

The third-party ccache does the same thing, the only difference being that it may additionally cache the output of each build.

szanni commented 2 years ago

Disabling ccache does work. This must be an issue with the lua wrapper though? Because running ccache directly does print warnings.

xmake v2.6.7 use builtin build cache instead of 3rd ccache command.

Compilers only print warnings when a file is compiled. If the file has not changed and therefore is not compiled: no warning printed.

What is still broken is xmake -wr. It should print warnings when rebuilding.

xmake/ccache will cache build object files to speed up rebuilding.

So when you re-build everything, xmake/ccache will also use the cached object files directly.

The third-party ccache does the same thing, the only difference being that it may additionally cache the output of each build.

That behavior seems extremely strange to me. If I specify -r or --rebuild, I expect it to reBUILD and not use some cache. Otherwise what is the point of that option? Relink? Then it should be called that. If this is the behavior of ccache too, then ccache is also broken. I am not terribly familiar with ccache, so I have not tested what it actually does.

waruqi commented 2 years ago

ccache and incremental compilation are two different features, if it is incremental compilation, then rebuild should recompile all of it.

ccache is mainly used to optimize the build speed of rebuild, even if rebuild all the code, it will still greatly improve the compilation speed by caching the obj files.

If you want to rebuild without using the cache at all, then you can disable it. xmake f --ccache=n, but I don't think it's necessary unless caching affects the normal build results, and I usually recommend turning it on.

szanni commented 2 years ago

I understand why using ccache is beneficial to compilation speed. But in this case it breaks xmake.

How is a user supposed to know, that --rebuild by default does NOT actually rebuild the code? ccache is enabled by default, therefore the -r switch is broken by default.

And I am aware that I can disable it via set_policy("build.ccache", false).

szanni commented 2 years ago

And the other problem I am having is that the -w switch is not enabled by default. At least I would need a set_policy("warning", true).

But for a build tool to not print warnings by default makes it unusable. Not printing warnings by default hides potential bugs.

waruqi commented 2 years ago

I understand why using ccache is beneficial to compilation speed. But in this case it breaks xmake.

If you think ccache is breaking something, you can open a separate issue and provide details of the error and steps to reproduce it. I will try to fix it.

How is a user supposed to know, that --rebuild by default does NOT actually rebuild the code?

The default output is clearly indicated, and you can see the ccache text in the progress message.

[  8%]: ccache compiling.debug src/tbox/libc/string/memcpy.c
[  8%]: ccache compiling.debug src/tbox/libc/string/strchr.c
[  8%]: ccache compiling.debug src/tbox/libc/string/wcscmp.c
[  8%]: ccache compiling.debug src/tbox/libc/string/wcslcpy.c
[  8%]: ccache compiling.debug src/tbox/libc/string/strncat.c

ccache is enabled by default, therefore the -r switch is broken by default.

As I said, ccache only makes sense when it is used at -r/--rebuild. If you don't use ccache at -r/--rebuild, it doesn't make any sense.

And the other problem I am having is that the -w switch is not enabled by default. At least I would need a set_policy("warning", true).

$ xmake g --build_warning=y

But for a build tool to not print warnings by default makes it unusable. Not printing warnings by default hides potential bugs.

By default, warnings are printed, and for most projects, the terminal will display back a lot of warning messages and we will not be able to view the compilation progress information properly.

This is especially noticeable with extra warnings enabled, and in some terminals with unfriendly window scrolling, such as cmd

waruqi commented 2 years ago

Also, for users, who simply need to compile the project, they do not pay much attention to the warning messages. It is not necessary to turn them on at all by default.

And for developers, it won't take much time to configure themselves to turn on warning output by default. xmake g --build_warning=y

and xmake-vscode/idea plugin will also enable xmake -w to build project for developers.

szanni commented 2 years ago
$ xmake g --build_warning=y

Again this applies only to my local computer. I need an option that is set in the file. So that anybody compiling the code sees the warnings. Anybody that types xmake.

By default, warnings are printed, and for most projects, the terminal will display back a lot of warning messages and we will not be able to view the compilation progress information properly.

This is especially noticeable with extra warnings enabled, and in some terminals with unfriendly window scrolling, such as cmd

I am wondering if this is a cultural difference? Because where I come from, anybody who compiles code is a developer. Normal user do not even know what a compiler is.

And if your code has so many warnings that you can not see the progress anymore, then your code most likely has many bugs. Warnings are very often hidden bugs.

The projects I work on all have a 0 warning policy. Even the windows kernel started using -Werror.

waruqi commented 2 years ago

The projects I work on all have a 0 warning policy. Even the windows kernel started using -Werror.

So you should use set_warnings("all", "error"). I also use the 0 warning policy, but I would leave it as a compile error.

https://github.com/xmake-io/xmake/blob/91395ec93c42631a613a64fd1b11ab6610bf3ea4/core/xmake.lua#L10-L11

szanni commented 2 years ago

So you should use set_warnings("all", "error"). I also use the 0 warning policy, but I would leave it as a compile error.

Not feasible, as this is a library being compiled on various platforms with various compilers. Breaking builds is just not an option.

I guess I give up and will have to stay with seemingly inferior build systems like CMake and Meson. I was hoping to adopt xmake, but no default warning option is sadly not acceptable for me. Which is sad, because I really like it.

Thank you for the effort nonetheless.

SirLynix commented 2 years ago

but no default warning option is sadly not acceptable for me

I'm really not sure to understand where the problem of adding -w is.

Most devs who will just type "xmake" to build the lib won't care about warnings in my opinion, all they want is the final product: binaries. I had a fair share of non-dev users doing this just to build and use one of my product.

Developers who want to contribute to the library will either:

This ain't much for a contributor, and I agree that if you find that warnings are so important, adding -Werror is an option, if the project doesn't build because a new warning appeared (which should be rare except if using some crazy option like -Weverything), why not add an option to disable -Werror which can be used when you need the library to build.

It's already what assimp does for example, enabling -Werror by default but providing an option to disable it for package managers.

eli-schwartz commented 2 years ago

Disabling ccache does work. This must be an issue with the lua wrapper though? Because running ccache directly does print warnings.

Indeed it does:

On a cache hit, ccache is able to supply all of the correct compiler outputs (including all warnings, dependency file, etc) from the cache.

...

How is a user supposed to know, that --rebuild by default does NOT actually rebuild the code? ccache is enabled by default, therefore the -r switch is broken by default.

In general, ccache is designed around the notion that every conceivable form of input (source files, compiler version, header includes, current locale, etc.) that might affect any part of the output, is hashed as part of the cache lookup key. The ccache man page goes into detail about how this works. It is supposed to err on the side of failing to return a cache hit in any case that the compiler would be able to return different output, even if that different output is just the warnings on stderr. (That is why it caches the locale too. There's an option to manually disable using the locale in the cache key and allow ccache to return warnings in the "wrong" locale.)

So although it's not technically a rebuild, it is semantically a rebuild, and acts exactly like a rebuild including what gets printed to the console.

... if you actually use ccache. If you don't use ccache, then the alternative tool needs to reimplement all the great work ccache has done, and possibly miss out on a lot of it.

With that in mind, I find it strange that people keep on talking about ccache.

This is due to the new version's built-in ccache, which you can disable to get the warning output. xmake f --ccache=n

So, it seems to me this is highly deceptive!!! Having a "ccache" option is a lie. I strongly recommend xmake renames this option to e.g. "xcache". This will provide visibility to users that if they have questions or issues with the functionality of the object caching, it is not the fault of ccache (because it is not being used).

Meanwhile I suppose that if people want "real ccache" then the traditional ccache method of defining the compiler as ccache gcc or ccache clang is the only way to go.

waruqi commented 2 years ago

I have added set_policy("build.warning", true) to enable warning output on dev branch. xmake update -s dev

And I have improved to get warnings when enable ccache. Even if the object file has been cached, each rebuild will also get the warning message.

please remove all cached files and try it again.

xmake clean --all

So, it seems to me this is highly deceptive!!! Having a "ccache" option is a lie. I strongly recommend xmake renames this option to e.g. "xcache". This will provide visibility to users that if they have questions or issues with the functionality of the object caching, it is not the fault of ccache (because it is not being used).

I know, It means c/c++ cache, and although it has the same name as the ccache utility, I don't want to modify it. Also, --distcc is the same, distributed c/c++ compilation.

Also, I've fixed the current warning problem ccache is having, and it keeps basically the same behavior as 3rd ccache, and both do the same thing.

szanni commented 2 years ago

In general, ccache is designed around the notion that every conceivable form of input (source files, compiler version, header includes, current locale, etc.) that might affect any part of the output, is hashed as part of the cache lookup key.

Wow, I was not aware. I hope it does that for files form /usr/include as well as hashes the compiler binaries and all related build tools as well?

So, it seems to me this is highly deceptive!!! Having a "ccache" option is a lie. I strongly recommend xmake renames this option to e.g. "xcache". This will provide visibility to users that if they have questions or issues with the functionality of the object caching, it is not the fault of ccache (because it is not being used).

Yes, this was very confusing to me too. Because I did not see a ccache dependency from the package manager. I hope this is not a "not invented here" syndrome thing. It seems like a pointless undertaking to replicate a build tool that works and is so complex.

I know, It means c/c++ cache, and although it has the same name as the ccache utility, I don't want to modify it. Also, --distcc is the same, distributed c/c++ compilation.

I do agree that it is extremely confusing. If I see ccache, I assume to get the actual ccache not some home baked alternative. It would be like calling ones operating system Windows and then claiming that made sense, because it displays windows.

I believe ccache does not actually stand for c/c++. The first line of the man page reads:

Ccache is a compiler cache.

I have added set_policy("build.warning", true) to enable warning output on dev branch. xmake update -s dev

Cool, thank you very much! Happy to see an option. This looks workable now.

Most devs who will just type "xmake" to build the lib won't care about warnings in my opinion, all they want is the final product: binaries. I had a fair share of non-dev users doing this just to build and use one of my product.

As I said, I guess this might be a cultural thing. I do not know a single non developer who would compile anything. They would give up at the word compile.

And I am not sure if this is true for everyone. For me the amount of warnings is a great indicator of code quality/development practices. If a library has more than a few warnings, I will most likely move on and try to find something else.

I the end it is about values for me. Are the values of the projects I rely upon aligned with my own. I value correct, bug free code over pretty output. Other people might have different values, and that is perfectly fine.

eli-schwartz commented 2 years ago

Wow, I was not aware. I hope it does that for files form /usr/include as well as hashes the compiler binaries and all related build tools as well?

Yes, it does.

waruqi commented 2 years ago

I think all the problems here should have been solved, and I won't consider changing the name of ccache for now. If you have any other questions, you can open a new issue.

OuYangPaste commented 2 years ago

I think it is possible to have both xcache and ccache options? Default with xcache, users can switch back to ccache (auto-close xcache)