Skip to content
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.

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

BLD: move -std=c99 addition to CFLAGS to Azure config #12620

Merged
merged 2 commits into from
Jan 10, 2019

Conversation

rgommers
Copy link
Member

See gh-12610 for details. Note, I removed this for TravisCI for now as a test, I want to see how big a problem we have for building from source at the moment.

@rgommers
Copy link
Member Author

The two TravisCI build matrix entry that have dist: xenial (= Ubuntu 16.04) passed, everything else failed on compiling pocketfft. Xenial on TravisCI has gcc 5.4.0. The other builds are on Ubuntu 14.04, and use gcc 4.8.4.

@rgommers
Copy link
Member Author

gcc 5.1 was only released in 2015; I think gcc 4.8 and 4.9 are still fairly extensively used. So only putting -std=c99 in our CI config may lead to more contributors and users running into this. On the other hand, we don't mess with inserting flags for other compilers either, I think we in general expect users/contributors/packagers to handle this by themselves. So not yet sure what the best thing to do is here.

@tylerjereddy
Copy link
Contributor

Seems to me like trying to automagically set compiler flags properly for people in the vast array of scenarios out there is asking for trouble compared to just documenting that we use C99 standard moving forward & directing issue reports to some brief documentation about this.

So, I think I like the idea of just setting the C99 flags where needed in CI & documenting the requirement briefly (which is probably already done somewhere) as opposed to a programmatic interjection on compiler flags for pip, setup.py & so on.

@charris
Copy link
Member

charris commented Jan 4, 2019

Note that -std=c99 is required for in loop variable declaration. It is needed for pocketfft, at least for the gcc version used to build wheels.

@rgommers
Copy link
Member Author

rgommers commented Jan 4, 2019

Note that -std=c99 is required for in loop variable declaration. It is needed for pocketfft, at least for the gcc version used to build wheels.

Yes I know. It's exactly because it's required that I think that we should not add it to runtests.py. Otherwise we get into the situation where we fix it for ourselves, and leave everyone else who installs from source to deal with the problems.

Instead, I think what we should do is one of:

  • document the correct solutions, and let everyone who uses a too old compiler deal with it themselves
  • hack it into numpy.distutils or a setup.py file to fix it for everyone

Since there's not much precedent and it's not entirely clear how big a fraction of users/devs will be impacted, I'm not sure which of the two options is best. I think putting it in numpy/setup.py is probably cleanest. Will need to make that from what's basically a do-nothing file into one that messes with compiler detection and CFLAGS then. The alternative is special-casing building numpy in numpy/distutils/ccompiler.py.

@rgommers
Copy link
Member Author

rgommers commented Jan 7, 2019

hack it into numpy.distutils or a setup.py file to fix it for everyone

Okay I tried, there's no good way to do either as far as I can tell: rgommers@8aaf4d8. I think just moving it from runtests.py to the Azure config as in the first commit in this PR is the way to go. gcc < 5.0 will become less common over time

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks sensible to me

@rgommers rgommers changed the title WIP: BLD: move -std=c99 addition to CFLAGS to Azure config BLD: move -std=c99 addition to CFLAGS to Azure config Jan 7, 2019
@eric-wieser
Copy link
Member

Is there any distutils parallel to autoconf's AC_PROG_CC_STDC or cmake's set_property(TARGET tgt PROPERTY C_STANDARD 99)?

@rgommers
Copy link
Member Author

rgommers commented Jan 7, 2019

@eric-wieser pretty sure there's not. That's why I tried to implement the check in the commit linked in the comment above.

@charris charris merged commit feb7ee0 into numpy:master Jan 10, 2019
@charris
Copy link
Member

charris commented Jan 10, 2019

Thanks Ralf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants