nicklan / pnmixer

Volume mixer for the system tray
GNU General Public License v3.0
152 stars 32 forks source link

Debug #107

Closed elboulangero closed 8 years ago

elboulangero commented 8 years ago

Hi,

just a word about CFLAGS: I find it much more convenient if the default CFLAGS include GCC warnings and all that stuff. So that I don't have to pass it each time I execute configure (which happens quite often). Autotools allow to define the default CFLAGS, so that is what I do in dabe4fa.

The CFLAGS in this commit are the one I usually use, but you may have different habits, so maybe we can agree on a set of default CFLAGS that make everyone happy. I usually also enable -Werror, but I think PNMixer is not ready for that :smile:

For packaging, I think it's no problem, packagers can always define their own CFLAGS at configure time.

Tell me if you're OK with that.

Apart from that, :+1: for merging to master

hasufell commented 8 years ago

So that I don't have to pass it each time I execute configure

You can just export them:

export CFLAGS="-g -O2 -Wall -Wextra"
./configure
make
make clean
./configure
...

You can even do the export in your bashrc...

IMO only CFLAGS that are required to make the project build should be defined by the build system. But I have no strong opinion on that.

elboulangero commented 8 years ago

But I have no strong opinion on that.

Me neither. OK let's keep it simple, no CFLAGS defined, I will do with the export as you suggest. I never realized I could do it that way.

hasufell commented 8 years ago

unrelated question: can we make autoconf procude a config.h file instead of appending all the -DFOO to the compiler line? I am writing a .ycm_extra_conf.py for use with youcompleteme, but adding all the defines manually is tedious.

elboulangero commented 8 years ago

I never used that, but I think I read about that lately

elboulangero commented 8 years ago

https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/Configuration-Headers.html

hasufell commented 8 years ago

You will need the following CFLAGS for pnmixer so that configure does not break:

CFLAGS="-O2 -g -Wall -Wextra -Werror -Wno-incompatible-library-redeclaration -Wno-uninitialized -Wno-deprecated-declarations -Wno-unused-parameter"

Currently running travis build for those: https://travis-ci.org/nicklan/pnmixer/builds/90980321

elboulangero commented 8 years ago

Doesn't work with gcc

cc1: error: unrecognized command line option ‘-Wno-incompatible-library-redeclarations’
hasufell commented 8 years ago

for gcc it is

CFLAGS="-O2 -g -fno-builtin -Wall -Wextra -Werror -Wno-uninitialized -Wno-deprecated-declarations -Wno-unused-parameter"
elboulangero commented 8 years ago

Configure breaks because of the AC_CHECK_LIB for libm. The test generate a C file and uses ceil function, but defines it with a wrong type. Here is the C file generated (as seen in config.log):

/* confdefs.h */
#define PACKAGE_NAME "pnmixer"
#define PACKAGE_TARNAME "pnmixer"
#define PACKAGE_VERSION "0.6.1"
#define PACKAGE_STRING "pnmixer 0.6.1"
#define PACKAGE_BUGREPORT ""
#define PACKAGE_URL ""
#define PACKAGE "pnmixer"
#define VERSION "0.6.1"
#define GETTEXT_PACKAGE "pnmixer"
#define STDC_HEADERS 1
#define HAVE_SYS_TYPES_H 1
#define HAVE_SYS_STAT_H 1
#define HAVE_STDLIB_H 1
#define HAVE_STRING_H 1
#define HAVE_MEMORY_H 1
#define HAVE_STRINGS_H 1
#define HAVE_INTTYPES_H 1
#define HAVE_STDINT_H 1
#define HAVE_UNISTD_H 1
#define HAVE_LOCALE_H 1
#define HAVE_LC_MESSAGES 1
#define HAVE_BIND_TEXTDOMAIN_CODESET 1
#define HAVE_GETTEXT 1
#define ENABLE_NLS 1
/* end confdefs.h.  */

/* Override any GCC internal prototype to avoid an error.
   Use char because int might match the return type of a GCC
   builtin and then its argument prototype would still apply.  */
#ifdef __cplusplus
extern "C"
#endif
char ceil ();
int
main ()
{
return ceil ();
  ;
  return 0;
}

Compiling that stuff leads to:

$ gcc main.c -o main -g -O2 -Wall -Wextra -Wshadow -Werror
main.c:34:6: error: conflicting types for built-in function ‘ceil’ [-Werror]
 char ceil ();
      ^
cc1: all warnings being treated as errors

For the moment, the only solution I found is to add -fno-builtin-ceilto the CFLAGS. Pretty ugly, I'm curious about how this situation should be handled...

elboulangero commented 8 years ago

Just read you're post. Yep, the -fno-builtin saves the configure

hasufell commented 8 years ago

Autotools configure isn't particularly compatible with the -Werror flag. That's a known problem. As soon as -Werror is dropped, all those problems are gone.

hasufell commented 8 years ago

In the end... Werror isn't that useful (except for the travis CI), since you can decide yourself whether or not the shown warnings are really important.

elboulangero commented 8 years ago

Hehe, the challenge is to enable the more warnings possible ! That's usually what I do. It's just a little bit annoying with the unused parameters, which happen a lot when developing stuff. But it forces you to remain very clean in your writing, so I consider this as a good habit.

But here, the main problem is the silly configure test for libm. I'm sure there's a nice solution for that.

hasufell commented 8 years ago

I fixed the unused parameters in https://github.com/nicklan/pnmixer/pull/111

elboulangero commented 8 years ago

We're done with, aren't we ?