Skip to content

-Werror=declaration-after-statement is added even for extension modules through setup.py #65320

Closed
Closed
-Werror=declaration-after-statement is added even for extension modules through setup.py#65320
Labels
buildThe build process and cross-buildextension-modulesC modules in the Modules dirstdlibStandard Library Python modules in the Lib/ directory

Description

@nilsge
nilsge
mannequin
opened on Apr 1, 2014
BPO 21121
Nosy @ronaldoussoren, @pitrou, @larryhastings, @benjaminp, @ned-deily, @merwok, @skrah, @dstufft, @wm75
Files
  • pybug.txt: setup.py and also the error log
  • issue21121.diff
  • issue21121-2.diff: improved version of issue21121.diff
  • issue21121-3.diff: alternative approach
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2014-08-10.03:04:30.723>
created_at = <Date 2014-04-01.10:42:13.877>
labels = ['extension-modules', 'build', 'library']
title = '-Werror=declaration-after-statement is added even for extension modules through setup.py'
updated_at = <Date 2014-08-10.03:04:30.712>
user = 'https://bugs.python.org/nilsge'

    bugs.python.org fields:

    activity = <Date 2014-08-10.03:04:30.712>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2014-08-10.03:04:30.723>
closer = 'python-dev'
components = ['Build', 'Distutils', 'Extension Modules']
creation = <Date 2014-04-01.10:42:13.877>
creator = 'nilsge'
dependencies = []
files = ['34692', '34909', '35133', '35134']
hgrepos = []
issue_num = 21121
keywords = ['patch']
message_count = 22.0
messages = ['215305', '215348', '215350', '216467', '216476', '216879', '217781', '217805', '217842', '217847', '217865', '217869', '217870', '217881', '217882', '217888', '217891', '217915', '218000', '218001', '224736', '225122']
nosy_count = 12.0
nosy_names = ['ronaldoussoren', 'pitrou', 'larry', 'benjamin.peterson', 'ned.deily', 'eric.araujo', 'Arfrever', 'skrah', 'python-dev', 'dstufft', 'wolma', 'nilsge']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue21121'
versions = ['Python 3.4', 'Python 3.5']

    Activity

    1. nilsge commented on Apr 1, 2014

      @nilsge
      nilsgemannequin
      on Apr 1, 2014
      MannequinAuthor

      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.

    2. nilsge
      added
      buildThe build process and cross-build
      stdlibStandard Library Python modules in the Lib/ directory
      extension-modulesC modules in the Modules dir
      on Apr 1, 2014

    3. ned-deily commented on Apr 2, 2014

      @ned-deily
      ned-deily
      on Apr 2, 2014
      Member

      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.)

    4. nilsge commented on Apr 2, 2014

      @nilsge
      nilsgemannequin
      on Apr 2, 2014
      MannequinAuthor

      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",""))')

    5. skrah commented on Apr 16, 2014

      @skrah
      skrahmannequin
      on Apr 16, 2014
      Mannequin

      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.

    6. wm75 commented on Apr 16, 2014

      @wm75
      wm75mannequin
      on Apr 16, 2014
      Mannequin

      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 .

    7. ned-deily commented on Apr 19, 2014

      @ned-deily
      ned-deily
      on Apr 19, 2014
      Member

      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.

    8. skrah commented on May 2, 2014

      @skrah
      skrahmannequin
      on May 2, 2014
      Mannequin

      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 bpo-21121-3.diff. Thoughts?

    9. merwok commented on May 2, 2014

      @merwok
      merwok
      on May 2, 2014
      Member

      I like bpo-21121-3.diff.

    10. ned-deily commented on May 3, 2014

      @ned-deily
      ned-deily
      on May 3, 2014
      Member

      I agree: bpo-21121-3.diff is a much better approach.

    11. larryhastings commented on May 3, 2014

      @larryhastings
      larryhastings
      on May 3, 2014
      Contributor

      If you guys want this in 3.4.1, please get it checked in in the next, oh, eight hours.

    12. skrah commented on May 4, 2014

      @skrah
      skrahmannequin
      on May 4, 2014
      Mannequin

      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).

    13. larryhastings commented on May 4, 2014

      @larryhastings
      larryhastings
      on May 4, 2014
      Contributor

      Sorry, I should have said "3.4.1rc1". You can still get it in for 3.4.1.

    14. ronaldoussoren commented on May 4, 2014

      @ronaldoussoren
      ronaldoussoren
      on May 4, 2014
      Contributor

      This is the same issue as bpo-18211. As that issue doesn't have a patch and this one does, I'm closing bpo-18211 as a duplicate.

    15. merwok commented on May 4, 2014

      @merwok
      merwok
      on May 4, 2014
      Member

      I can commit the patch but won’t be able to check the buildbots for the next twelve hours.

    16. ned-deily commented on May 4, 2014

      @ned-deily
      ned-deily
      on May 4, 2014
      Member

      There's no immediate rush now. It's too late for 3.4.1rc1.

    17. skrah commented on May 4, 2014

      @skrah
      skrahmannequin
      on May 4, 2014
      Mannequin

      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.

    18. ned-deily commented on May 4, 2014

      @ned-deily
      ned-deily
      on May 4, 2014
      Member

      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().

    19. skrah commented on May 5, 2014

      @skrah
      skrahmannequin
      on May 5, 2014
      Mannequin

      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.

    20. ned-deily commented on May 6, 2014

      @ned-deily
      ned-deily
      on May 6, 2014
      Member

      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.)

    21. ronaldoussoren commented on May 6, 2014

      @ronaldoussoren
      ronaldoussoren
      on May 6, 2014
      Contributor

      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.

    22. pitrou commented on Aug 4, 2014

      @pitrou
      pitrou
      on Aug 4, 2014
      Member

      This patch should definitely go in.

    23. python-dev commented on Aug 10, 2014

      @python-dev
      python-devmannequin
      on Aug 10, 2014
      Mannequin

      New changeset 2a3538f14948 by Benjamin Peterson in branch '3.4':
      add -Werror=declaration-after-statement only to stdlib extension modules (closes bpo-21121)
      http://hg.python.org/cpython/rev/2a3538f14948

      New changeset a5368cfbea0e by Benjamin Peterson in branch 'default':
      merge 3.4 (bpo-21121)
      http://hg.python.org/cpython/rev/a5368cfbea0e

    24. python-dev
      closed this as completedon Aug 10, 2014
    25. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      buildThe build process and cross-buildextension-modulesC modules in the Modules dirstdlibStandard Library Python modules in the Lib/ directory

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions