8000 TST: Update runtests.py to specify C99 for gcc. by charris · Pull Request #12610 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

TST: Update runtests.py to specify C99 for gcc. #12610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 25, 2018
Merged

TST: Update runtests.py to specify C99 for gcc. #12610

merged 1 commit into from
Dec 25, 2018

Conversation

charris
Copy link
Member
@charris charris commented Dec 25, 2018

Prefix -std=c99 to the CFLAGS environmental variable. C99 is the
standard for NumPy >= 1.17.

@charris
TST: Update runtests.py to specify C99 for gcc.
1709249 
Prefix `-std=c99` to the CFLAGS environmental variable. C99 is the
standard for NumPy >= 1.17.
@charris charris mentioned this pull request Dec 25, 2018
Merged
@charris
Copy link
Member Author
charris commented Dec 25, 2018

I'm just going to put this in as it is needed for #11888.

@charris charris merged commit 09dd75e into numpy:master Dec 25, 2018
@rgommers
Copy link
Member

@charris this flag shouldn't be added here I think. If it's needed then it's needed also when compiling without runtests.py. Or is it specifically only needed because of the -Werror flags we add in runtests.py?

@charris
Copy link
Member Author
charris commented Dec 26, 2018

It is needed because runtests.py is used in the CI testing. In particular, the pocketfft tests would fail on azure. Note that we require C99 for NumPy >= 1.17.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

It may be that it isn't required after removing the -Wdeclaration-after-statement, but we are testing with C99 on travis and I didn't see a reason not to do so with runtests.py.

@eric-wieser
Copy link
Member

I do wonder whether removing this for 1.17 is worth the extra pain it will incur for 1.16.x backports. Do we have any kind of strategy for what we should do now and what we should save for 1.18?

@rgommers
Copy link
Member

but we are testing with C99 on travis and I didn't see a reason not to do so with runtests.py.

this is different than adding -Werror flags though. If we only test with -std=c99, we will easily miss build problems that most users will see when they install from source with pip or setuptools. This should go into a setup.py file it looks like.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

Note that runtest prepends to CFLAGS, so I also wanted to keep C99 first in the list so it could be overridden by those flags.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

setup.py file it looks like.

That seems reasonable, although it needs to be compiler dependent.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

Note that the option of a c99 ccompiler in distutils was overridden.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

So, thoughts on how to put it into the setup file? I think we would need to use distutils for that or put it into the environment, and the latter would make it last in the flags used by runtest unless we check for it and change the order.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

I do wonder whether removing this for 1.17 is worth the extra pain it will incur for 1.16.x backports.

That is a potential problem, but it is definitely needed for pocketfft. I think we can live with it. I don't expect to backport anything both new and big and runtest is branch specific.

@rgommers
Copy link
Member

Note that the option of a c99 ccompiler in distutils was overridden.

What do you mean by this? I only see std=c99 in intelcompiler.py.

So, thoughts on how to put it into the setup file? I think we would need to use distutils for that or put it into the environment, and the latter would make it last in the flags used by runtest unless we check for it and change the order.

If we put it in distutils then we have to detect there whether we're compiling NumPy or something else. I think that's probably more fragile than in setup.py check which compiler we use and appending to the compile flags of the CCompiler instance.

@charris
Copy link
Member Author
charris commented Dec 26, 2018

What do you mean by this?

I mean that I raised the possibility of a c99 distutils compiler on the list (or maybe an earlier PR) and it wasn't popular. I was thinking of the c99 compiler that is standard on Unix systems.

charris@fc [numpy.git (master)]$ which c99
/usr/bin/c99

On my system, all that file does is prepend the -std=c99 flag to the existing CFLAGS.

@eric-wieser
Copy link
Member

I don't expect to backport anything both new and big and runtest is branch specific

Right, but bug fixes might be written using unnecessary c99 features, which will fail CI when backported. As long as backporters are happy to pay the cost of occasionally moving some variable declarations after a failed CI, that's not a problem.

@rgommers
Copy link
Member

As long as backporters are happy to pay the cost of occasionally moving some variable declarations after a failed CI, that's not a problem.

that seems fine/reasonable to me.

I mean that I raised the possibility of a c99 distutils compiler on the list (or maybe an earlier PR) and it wasn't popular. I was thinking of the c99 compiler that is standard on Unix systems.

Ah okay, that vaguely rings a bell. I don't really think that's warranted or necessary for adding a single flag. Besides, since it's not actually a separate compiler it is not something that's easy to auto-detect.

I'll try to come up with a simple patch for this.

@rgommers
Copy link
Member
rgommers commented Dec 27, 2018 •
edited
Loading

For reference, here is the failing 32-bit Linux CI run: https://dev.azure.com/numpy/numpy/_build/results?buildId=1373. It wasn't immediately clear to me why that one failed with a bunch of errors like:

2018-12-20T14:52:15.7666402Z i686-linux-gnu-gcc: numpy/fft/pocketfft.c
2018-12-20T14:52:15.7666663Z numpy/fft/pocketfft.c: In function 'my_sincosm1pi':
2018-12-20T14:52:15.7666976Z numpy/fft/pocketfft.c:54:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]

but all other builds succeeded.It turns out that from gcc 5.1 onwards, the default C mode is gnu11. From https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/Standards.html:

Use of the -std options listed above will disable these extensions where they conflict
with the C standard version selected. You may also select an extended version of 
the C language explicitly with -std=gnu90 (for C90 with GNU extensions), -std=gnu99
(for C99 with GNU extensions) or -std=gnu11 (for C11 with GNU extensions). The default,
if no C language dialect options are given, is -std=gnu11.

So changing this in runtests.py will actually downgrade the C mode from C11 to C99 for most people using runtests.py.

It's a bit odd that ubuntu-16.04, which we use in the problematic 32-bit Linux tests, ships gcc 5.3.1 by default but it doesn't work as advertised. Either way, I think my conclusion after looking into this is that it's reasonable to expect >=c99 behavior by default and to 8000 add -std=c99 explicitly only if needed. Hence moving it from runtests.py to the Azure build matrix entry seems fine. And we're going to have to document this in the release notes, because we'll probably see some issues on more exotic platforms.

EDIT: misread the Ubuntu package page, the default is gcc 4.5 with gcc 5.3 being an alternative available as the gcc-5 package

rgommers added a commit to rgommers/numpy that referenced this pull request Dec 27, 2018
@rgommers
BLD: move -std=c99 from runtests.py to Azure config
02e538a 
See numpygh-12610 for details.
@rgommers rgommers mentioned this pull request Dec 27, 2018
Merged
@saimn saimn mentioned this pull request Jan 2, 2019
Closed
@pllim
Copy link
Contributor
pllim commented Jan 6, 2019

Hi. This might have broken a Travis CI job over at Astropy, which tests against development version of Numpy (see astropy/astropy#8318). The Travis CI log is https://travis-ci.org/astropy/astropy/jobs/475320252 . I also opened an issue about it over at conda/conda#8058 . Any advice would be greatly appreciated. Thanks!

@charris
Copy link
Member Author
charris commented Jan 6, 2019

@pllim Yes, NumPy 1.17 built with gcc requires C99 for the loop variable declarations. Should be an easy fix on the astropy end. You will also need Python > 3.4 on Windows. Why are you building your own NumPy? There are nightly builds of NumPy wheels, admittedly not always successful :)

@charris
Copy link
Member Author
charris commented Jan 6, 2019

@rgommers You have convinced me that it probably better to add -std=c99 in those places, the CI for instance, that require it. I suppose the best test is "does it compile?"

@charris charris deleted the update-runtests-to-c99 branch January 6, 2019 02:23
@rgommers
Copy link
Member
rgommers commented Jan 6, 2019

CIs of other packages, which we encouraged to test against NumPy master, will all be broken now. So that's an argument for fixing it within the numpy build. I'll see what I can do this weekend.

@pllim
Copy link
Contributor
pllim commented Jan 6, 2019

Thank you all for the quick response! I am looking forward to a solution.

Should be an easy fix on the astropy end

I am not sure. I tried to grep for "C89" and got into rabbit holes that are libexpat and wcslib that are bundled with Astropy under cextern. A simple "copy and paste" of latest libexpat from the source into Astropy's bundle failed horribly (it is pretty clear from file listing diff that a big refactoring was done by libexpat). If the fix is indeed very simple and I was looking at the wrong place, then please feel free to open PR over at Astropy's side. I agree that Astropy should use a more modern C compiler, if possible.

Why are you building your own NumPy?

Indeed, @bsipocz had the same idea as you to use nightly wheels for Astropy's CI against development version of Numpy. Perhaps she had done it, even, and I simply did not realize. I could not find the PR over at astropy/ci-helpers that implemented the wheels (or at least it is not obvious from a GitHub search).

@bsipocz
Copy link
Member
bsipocz commented Jan 6, 2019

Why are you building your own NumPy?

Indeed, @bsipocz had the same idea as you to use nightly wheels for Astropy's CI against development version of Numpy. Perhaps she had done it, even, and I simply did not realize. I could not find the PR over at astropy/ci-helpers that implemented the wheels (or at least it is not obvious from a GitHub search).

We talked about in December and I haven't got to it yet since, but it's on the list of to-be-dones somewhere. Building numpy from source wasn't painful enough to require an emergency fix 😅
8000

@rgommers
Copy link
Member
rgommers commented Jan 6, 2019 •
edited
Loading

There isn't a need to change the AstroPy build. The CI script simply has to pass -std=c99 to the NumPy build. Probably by pip install numpy --install-option="-std=c99" (but then whatever you use to install numpy master instead of "numpy".

@pllim pllim mentioned this pull request Jan 6, 2019
Merged
@pllim
Copy link
Contributor
pllim commented Jan 6, 2019

Ah, thank you, @rgommers ! I was indeed looking at the wrong place. 😅

@pllim
Copy link
Contributor
pllim commented Jan 6, 2019

Tried the suggestion at astropy/ci-helpers#368 but it does not work. Let's move the discussion there.

@rgommers
Copy link
Member
rgommers commented Jan 6, 2019

That was a braindead suggestion, sorry. I'll comment here in case other people look for the solution here. In the TravisCI script, use:

export CFLAGS="-std=c99"

on the line above where you install numpy. setup.py doesn't understand -std or any such flag that's meant for gcc.

@pllim
Copy link
Contributor
pllim commented Jan 6, 2019

Yes, I can confirm that #12610 (comment) works. Thank you so much for your quick responses and over the weekend no less. Thank you!!!

rgommers added a commit to rgommers/numpy that referenced this pull request Jan 7, 2019
@rgommers
BLD: add -std=c99 to CFLAGS for gcc < 5.0
8aaf4d8 
Follow up to numpygh-12610.
rgommers added a commit to rgommers/numpy that referenced this pull request Jan 7, 2019
@rgommers
BLD: move -std=c99 from runtests.py to Azure config
a5c3579 
See numpygh-12610 for details.
@charris charris mentioned this pull request Aug 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
05 - Testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@charris @rgommers @eric-wieser @pllim @bsipocz
0