[Swig-devel] Warning option -Wdate-time requested by Debian reproducible builds project

Discussion:

Torsten Landschoff

2015-08-28 22:16:26 UTC

Hello,

I was recently asked by the Debian reproducible builds team if SWIG
could be extended to support the -Wdate-time warning option.

Actually the suggestion was to ignore all unknown -Wwhatever flags that
are not known to SWIG - see the full text here:

https://bugs.debian.org/790102

Hi subversion, unbound and swig maintainers,
we in the Reproducible Builds effort use a non default dpkg which export
-Wdate-time through dpkg-buildflags.
There are package that pass CPPFLAGS (for example) quite unchanged to swig.
Sadly swig does not recognize -Wdate-time and choke and fail on it badly, e.g.
/usr/bin/swig -I. -Wdate-time -D_FORTIFY_SOURCE=2 -I/usr/include/python2.7 -I/usr/include/python2.7 -o pythonmod/interface.h -python ./pythonmod/interface.i
swig error : Unrecognized option -Wdate-time
Use 'swig -help' for available options.
Makefile:369: recipe for target 'pythonmod/interface.h' failed
make[1]: *** [pythonmod/interface.h] Error 1
Needless to say, I'd like to build those packages with such flag (that we also
would like to get into the default set, see the bug #762683 for a start).
a. swig stops failing so badly on unrecognized options. If you really want to
validate the options at least stop failing on unrecognized -W? Seems quite
logical to pass CPPFLAGS to swig to me, and as such sounds sane
b. people stop passing CPPFLAGS to swig. I've already saw a "CPPFLAGS cleaner"
on subversion's configure script. Please I don't want to see another
Even if I do have preference I don't have a strong opinion on this, so if you
agree this is not a bug in swig (which I could even accept, given that the
accepted flags are well documented), I'll just clone+reassign this bug around.

IMHO SWIG should not just ignore any unknown warning setting, at least
not without making a bit of noise on stderr. Even than I'd rather have a
tool understand all options that I passed. So just swallowing all -W
options appears a bit over the top to me.

And of course gcc is not the only compiler used in together with SWIG so
where would that stop? Here are the warning options supported by GCC and
Visual Studio CL compilers:

https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/Warning-Options.html#Warning-Options
https://msdn.microsoft.com/en-us/library/vstudio/thxezb7y%28v=vs.100%29.aspx

Solution (b) would be better since we can not be sure that swig options
will not one day conflict with compiler options of the same name.

However, currently only -Wdate-time seems to pose a problem and swig
already accepts -Wall. Therefore I propose to make a pragmatic change to
allow -Wdate-time as well and postpone a discussion about handling of
random CPPFLAGS thrown at swig.

I just created a pull request which just adds -Wdate-time as an accepted
but ignored command line option:

https://github.com/swig/swig/pull/511

Greetings, Torsten

------------------------------------------------------------------------------

William S Fulton

2015-08-29 11:45:35 UTC

Permalink

Post by Torsten Landschoff
Hello,
I was recently asked by the Debian reproducible builds team if SWIG
could be extended to support the -Wdate-time warning option.
Actually the suggestion was to ignore all unknown -Wwhatever flags that
https://bugs.debian.org/790102

IMHO SWIG should not just ignore any unknown warning setting, at least
not without making a bit of noise on stderr. Even than I'd rather have a
tool understand all options that I passed. So just swallowing all -W
options appears a bit over the top to me.
And of course gcc is not the only compiler used in together with SWIG so
where would that stop? Here are the warning options supported by GCC and
https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/Warning-Options.html#Warning-Options
https://msdn.microsoft.com/en-us/library/vstudio/thxezb7y%28v=vs.100%29.aspx
Solution (b) would be better since we can not be sure that swig options
will not one day conflict with compiler options of the same name.
However, currently only -Wdate-time seems to pose a problem and swig
already accepts -Wall. Therefore I propose to make a pragmatic change to
allow -Wdate-time as well and postpone a discussion about handling of
random CPPFLAGS thrown at swig.
I just created a pull request which just adds -Wdate-time as an accepted
https://github.com/swig/swig/pull/511

I agree that it doesn't make sense for SWIG to try and swallow all -W
options or any other C/C++ compiler options that are irrelevant to
SWIG for that matter. I'd rather not accept -Wdate-time or any other
compatibility flags as they are not relevant to SWIG. However, in the
interest of helping the reproducible builds project, I suggest we add
in this flag as a one off exception.

William

------------------------------------------------------------------------------

Olly Betts

2015-09-14 20:18:48 UTC

Permalink

Post by Torsten Landschoff
I was recently asked by the Debian reproducible builds team if SWIG
could be extended to support the -Wdate-time warning option.

I'm sympathetic to reproducible builds goal (and have fixed a few
packages myself), but with my upstream SWIG maintainer hat on I don't
think the proposed change to SWIG is sensible.

I've already responded in the PR, but wsfulton (another of the SWIG
upstream maintainers) suggested I should bring it up here.

Post by Torsten Landschoff

a. swig stops failing so badly on unrecognized options. If you
really want to validate the options at least stop failing on
unrecognized -W? Seems quite logical to pass CPPFLAGS to swig to me,
and as such sounds sane

SWIG parses C and C++, so it needs to know about include paths and
pre-defined macros, and so it has command-line options to specify
these. For sanity, they're the same options as cpp uses (otherwise
everyone would need to pointlessly learn new options).

But fundamentally SWIG is not cpp, and doesn't aim to be a drop in
replacement. Even just considering GCC, there are many other command
line options to cpp which swig won't accept
(https://gcc.gnu.org/onlinedocs/cpp/Invocation.html#Invocation) and
patching SWIG to ignore them all doesn't seem a sensible approach.

I would say packages which do '$(SWIG) $(CPPFLAGS) [...]' or similar are
buggy, and the correct fix is to patch their build systems to not do
that.

Post by Torsten Landschoff
However, currently only -Wdate-time seems to pose a problem and swig
already accepts -Wall.

SWIG's -Wall isn't the same as GCC's though - GCC's turns on warnings
which aren't on by default, while SWIG's disables warning suppressions
(usually specified in the interface files). It's really a development
tool for checking what's being suppressed is what you intended to be
suppressed, and doesn't make much sense to enable during builds of
Debian packages. It effectively says "show me all the noise warnings" -
in GCC terms it's like making '#pragma GCC diagnostic ignored "..."' a
no-op.

Post by Torsten Landschoff
Therefore I propose to make a pragmatic change to
allow -Wdate-time as well and postpone a discussion about handling of
random CPPFLAGS thrown at swig.

Even as a pragmatic but wrong change this doesn't seem to be justified
- from what I gather there are two affected packages - subversion and
unbound. It's not like the effort to address this properly is enormous
compared to the effort to address it the wrong way.

It also doesn't seem helpful to postpone discussion of the general issue
while accepting the thin end of the wedge. Once we start accepting
patches to ignore cpp options, it's much harder to start saying "no" to
them.

Cheers,
Olly

------------------------------------------------------------------------------

Torsten Landschoff

2015-09-21 04:01:22 UTC

Permalink

Hi Olly,

Post by Olly Betts
I've already responded in the PR, but wsfulton (another of the SWIG
upstream maintainers) suggested I should bring it up here.

which makes sense I think because it is still easier to track email than
ongoing discussions on github.

Post by Olly Betts
SWIG parses C and C++, so it needs to know about include paths and
pre-defined macros, and so it has command-line options to specify
these. For sanity, they're the same options as cpp uses (otherwise
everyone would need to pointlessly learn new options).

Fully agreed. I am now wondering if -I<dir> is really comparable to the
path passed to cpp, since the include directory of swig should contain
SWIG include files (*.i), not header files (*.h). The documentation
unfortunately says very little about the intended use, but there is an
example of adding -I/usr/include to build wrappers for the C library.

One question remains though: How can SWIG know of the configuration of
wrapped libraries? It is not unusual to have a config.h header and it
would be quite helpful to have the defines available in SWIG to avoid
wrapping unavailable functions that may cause linker errors. This is of
course a separate issue, but I think it is related as the config headers
usually need an extra include path defined via -I.

Post by Olly Betts
replacement. Even just considering GCC, there are many other command
line options to cpp which swig won't accept
(https://gcc.gnu.org/onlinedocs/cpp/Invocation.html#Invocation) and
patching SWIG to ignore them all doesn't seem a sensible approach.

Definitely.

Post by Olly Betts
I would say packages which do '$(SWIG) $(CPPFLAGS) [...]' or similar are
buggy, and the correct fix is to patch their build systems to not do
that.

Which brings up the question of what is the replacement. Under the
condition that I fill CPPFLAGS etc. manually there is no big deal. But
usually you collect defines, include paths etc. from config scripts
(xslt-config, python-config etc.) or from pkg-config.

Post by Olly Betts

Post by Torsten Landschoff
However, currently only -Wdate-time seems to pose a problem and swig
already accepts -Wall.

Good to know. I have to admit that I did not read the documentation
about it and expected -Wall to behave like gcc.

Post by Olly Betts

Post by Torsten Landschoff
Therefore I propose to make a pragmatic change to
allow -Wdate-time as well and postpone a discussion about handling of
random CPPFLAGS thrown at swig.

Even as a pragmatic but wrong change this doesn't seem to be justified
- from what I gather there are two affected packages - subversion and
unbound. It's not like the effort to address this properly is enormous
compared to the effort to address it the wrong way.
It also doesn't seem helpful to postpone discussion of the general issue
while accepting the thin end of the wedge. Once we start accepting
patches to ignore cpp options, it's much harder to start saying "no" to
them.

Well said. I concur.
I'll retract the pull request and will check the relevant packages
instead to search for a better solution.

I have to admint that I had a big thinko: There are thousands of
packages in the archive and patching them all to pass the correct flags
to SWIG would be a nightmare.

But there are only about 150 packages that build depend on swig, so this
should be much easier to fix.

Greetings, Torsten

------------------------------------------------------------------------------

Olly Betts

2015-09-21 05:41:35 UTC

Permalink

Post by Torsten Landschoff

SWIG doesn't follow '#include' by default (you can pass -includeall and
it will). So by default you're telling it where to find .i files, plus
any headers which the interface files explicitly include using %include.

If you use -includeall, I think that uses the same search path. Looking
at codesearch, -includeall seems to be use with SWIG by about 11 debian
packages (ignoring versions of SWIG).

Post by Torsten Landschoff
One question remains though: How can SWIG know of the configuration of
wrapped libraries? It is not unusual to have a config.h header and it
would be quite helpful to have the defines available in SWIG to avoid
wrapping unavailable functions that may cause linker errors. This is of
course a separate issue, but I think it is related as the config headers
usually need an extra include path defined via -I.

I'm not sure I follow. SWIG wraps the API declared in the header you
point it at, while config.h is a build-time header allowing you to use
the results of configure tests in C preprocessor tests.

Post by Torsten Landschoff
Which brings up the question of what is the replacement. Under the
condition that I fill CPPFLAGS etc. manually there is no big deal. But
usually you collect defines, include paths etc. from config scripts
(xslt-config, python-config etc.) or from pkg-config.

Your examples are actually quite enlightening if you look at them
in detail. Generally you can't easily fill CPPFLAGS from *-config!

Neither xslt-config nor python-config seem to provide any way to get
just CPPFLAGS, only --cflags. This seems to be the norm for *-config
scripts - whichever the original one was didn't split CPPFLAGS out from
CFLAGS, so very few of them do (the only exception I am aware of is
wxWidgets' wx-config, which does support --cppflags - possibly so you
can easily pass them to the resource compiler under mingw).

And pkg-config also has no obvious way to get CPPFLAGS as such either.
It does allow you to ask for just the -I flags with --cflags-only-I, but
there's no obvious way to get -D, -U or any other preprocessor flags
without filtering them from --cflags. And you can probably safely pass
the -I options to SWIG anyway.

Cheers,
Olly

------------------------------------------------------------------------------