classification
Title: -Werror=declaration-after-statement is added even for extension modules through setup.py
Type: Stage: resolved
Components: Build, Distutils, Extension Modules Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, benjamin.peterson, dstufft, eric.araujo, larry, ned.deily, nilsge, pitrou, python-dev, ronaldoussoren, skrah, wolma
Priority: high Keywords: patch

Created on 2014-04-01 10:42 by nilsge, last changed 2014-08-10 03:04 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
pybug.txt nilsge, 2014-04-01 10:42 setup.py and also the error log
issue21121.diff skrah, 2014-04-16 13:58 review
issue21121-2.diff skrah, 2014-05-02 19:58 improved version of issue21121.diff review
issue21121-3.diff skrah, 2014-05-02 19:58 alternative approach review
Messages (22)
msg215305 - (view) Author: nils (nilsge) Date: 2014-04-01 10:42
I got an error while rebuilding a module for 3.4. This was a ISO C90 error but setup.py explicitely adds "-std=c99" to the gcc parameters, and indeed it is used. 

fifo.h:114:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
uint32_t ofs = fifo->write_count - fifo->write_offset; 

However, Py 3.4 seems to add -Werror=declaration-after-statement also for extension modules. This should not happe (said also Yhg1s in #python).

Attached is a file that shows the setup.py and also the error log.
msg215348 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-04-02 01:01
It looks like -Werror=declaration-after-statement was added to BASECFLAGS in configure.ac during the 3.4 development cycle by a3559c8c614b and e47806951fb2. Unfortunately, BASECFLAGS propagates through to extension module builds as well.  If -Werror=declaration-after-statement should only be restricted to the build of the interpreter executable itself, one option *might be* to move the test and definition to CFLAGSFORSHARED in configure.ac.

A workaround could be to define CFLAGS before rebuilding a module:

export CFLAGS=$(python3.4 -c 'import sysconfig; print(sysconfig.get_config_var("CFLAGS").replace("-Werror=declaration-after-statement",""))')

(As usual, my brain hurts after trying to sift through the myriad build flags and their interactions in configure.ac, Makefile, and setup.py.)
msg215350 - (view) Author: nils (nilsge) Date: 2014-04-02 02:25
The workaround indeed works. At least I can work now until this gets officialy fixed. Thanks! 

export CFLAGS=$(python3.4 -c 'import sysconfig; print(sysconfig.get_config_var("CFLAGS").replace("-Werror=declaration-after-statement",""))')
msg216467 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-04-16 13:58
Here is a patch.  I do not see a really nice way to deal with the problem.
The cleanest way I found was to introduce a new Makefile variable CFLAGS_NODIST
and use that in the interpreter and stdlib build.
msg216476 - (view) Author: Wolfgang Maier (wolma) * Date: 2014-04-16 14:59
I ran into this issue right after 3.4 got released.

I solved it by adding

extra_compile_args=["-Wno-error=declaration-after-statement"]

as an argument to the Extension() call in the package's setup.py .
msg216879 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-04-19 21:16
Stefan, the patch LGTM although I sure wish we were removing some CFLAGS-related configuration variables rather than adding another.  But I don't have a better suggestion short of a comprehensive cleanup of all of them.
msg217781 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-05-02 19:58
Thanks, Ned.  I'm attaching a second version of the existing patch
with improved error handling and a fix for test_distutils, which
failed.

The result is slightly overcomplicated, so I came up with a
different approach in issue21121-3.diff.  Thoughts?
msg217805 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-05-02 22:35
I like issue21121-3.diff.
msg217842 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-05-03 23:09
I agree: issue21121-3.diff is a much better approach.
msg217847 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-05-03 23:40
If you guys want this in 3.4.1, please get it checked in in the next, oh, eight hours.
msg217865 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-05-04 10:37
> If you guys want this in 3.4.1, please get it checked in in the next, oh, eight hours.

I can't commit today.  Perhaps one of you wants to take over (I think we all
agree that the third patch is the best).
msg217869 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-05-04 11:52
Sorry, I should have said "3.4.1rc1".  You can still get it in for 3.4.1.
msg217870 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2014-05-04 12:34
This is the same issue as Issue18211. As that issue doesn't have a patch and this one does, I'm closing Issue18211 as a duplicate.
msg217881 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-05-04 18:01
I can commit the patch but won’t be able to check the buildbots for the next twelve hours.
msg217882 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-05-04 18:06
There's no immediate rush now. It's too late for 3.4.1rc1.
msg217888 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-05-04 20:15
One more question:

I think it's nicer to add CFLAGS_NODIST to 'renamed_variables' in
Lib/sysconfig.py:265:

    renamed_variables = ('CFLAGS', 'CFLAGS_NODIST', 'LDFLAGS', 'CPPFLAGS')

That way it's possible to look up CFLAGS_NODIST directly.



For consistency, we can do the same for Lib/distutils/sysconfig.py,
where the variable would be purely informational.
msg217891 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-05-04 20:45
Is there any reason to expose CFLAGS_NODIST externally?  It seems to me that it is only needed in the top-level setup.py for building standard library extension modules.  Let's not add yet another configuration variable to the already confusing array we present to users through the two sysconfig.get_config_var().
msg217915 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-05-05 09:31
The current patch allows the user to specify e.g.:

CFLAGS_NODIST="-march=core2" ./configure


So it would be surprising to get:

>>> import sysconfig
>>> sysconfig.get_config_var('CFLAGS_NODIST')
''


Now, we could restrict ourselves entirely to internal PY_CFLAGS_NODIST,
but I think exposing the feature is really useful if users or
distributors want to specify optimizations, FPU behavior or other
things that should not generally show up in distutils.
msg218000 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-05-06 18:29
I don't have a *really* strong opinion against it.  It's just that I find the current plethora of configuration flags to be non-intuitive and confusing (and there are plenty of open issues agreeing with that sentiment) and adding another with the name CFLAGS_NODIST doesn't help.  But, again, short of someone going in and doing a radical simplification of the whole configure.ac, Makefile.pre.in, and setup.py tangle, I guess exposing one more variable isn't going to make matters that much worse than they already are and it does solve a real problem.  (Sorry to vent on your patch, Stefan.)
msg218001 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2014-05-06 18:42
I share Ned's sentiment on this. The amount of information exposed through sysconfig is too large as it is because a lot of that information is only useful during the built of python itself. I'm pretty sure that have been patches in the past where users tried to use some of those variables and were surprised they didn't work for an installed python.

Anyways, the patch looks good.
msg224736 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-08-04 15:36
This patch should definitely go in.
msg225122 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-10 03:04
New changeset 2a3538f14948 by Benjamin Peterson in branch '3.4':
add -Werror=declaration-after-statement only to stdlib extension modules (closes #21121)
http://hg.python.org/cpython/rev/2a3538f14948

New changeset a5368cfbea0e by Benjamin Peterson in branch 'default':
merge 3.4 (#21121)
http://hg.python.org/cpython/rev/a5368cfbea0e
History
Date User Action Args
2014-08-10 03:04:30python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg225122

resolution: fixed
stage: commit review -> resolved
2014-08-04 15:36:56pitrousetnosy: + pitrou
messages: + msg224736
2014-05-11 11:59:42Arfreversetnosy: + Arfrever
2014-05-06 18:42:24ronaldoussorensetmessages: + msg218001
2014-05-06 18:29:15ned.deilysetmessages: + msg218000
2014-05-05 09:31:21skrahsetmessages: + msg217915
2014-05-04 20:45:42ned.deilysetmessages: + msg217891
2014-05-04 20:15:39skrahsetmessages: + msg217888
2014-05-04 18:06:33ned.deilysetmessages: + msg217882
2014-05-04 18:01:57eric.araujosetmessages: + msg217881
2014-05-04 12:40:21ronaldoussorenlinkissue18211 superseder
2014-05-04 12:34:32ronaldoussorensetnosy: + ronaldoussoren
messages: + msg217870
2014-05-04 11:52:33larrysetmessages: + msg217869
2014-05-04 10:37:55skrahsetmessages: + msg217865
2014-05-03 23:40:56larrysetmessages: + msg217847
2014-05-03 23:09:20ned.deilysetmessages: + msg217842
2014-05-02 22:35:41eric.araujosetmessages: + msg217805
2014-05-02 19:58:55skrahsetfiles: + issue21121-3.diff
2014-05-02 19:58:39skrahsetfiles: + issue21121-2.diff

messages: + msg217781
2014-04-19 21:16:30ned.deilysetstage: commit review
messages: + msg216879
versions: + Python 3.5
2014-04-16 14:59:50wolmasetnosy: + wolma
messages: + msg216476
2014-04-16 14:06:53skrahlinkissue21167 dependencies
2014-04-16 13:58:03skrahsetfiles: + issue21121.diff

nosy: + skrah
messages: + msg216467

keywords: + patch
2014-04-02 02:25:10nilsgesetmessages: + msg215350
2014-04-02 01:01:51ned.deilysetnosy: + ned.deily, benjamin.peterson
messages: + msg215348
2014-04-01 14:01:48skrahsetpriority: normal -> high
nosy: + larry
2014-04-01 10:42:13nilsgecreate