Skip to content

TST, BUILD: add a gcc 4.8 run on ubuntu 18.04 - #14771

Merged
charris merged 7 commits into
numpy:masterfrom
mattip:gcc-4.8
Oct 26, 2019
Merged

TST, BUILD: add a gcc 4.8 run on ubuntu 18.04#14771
charris merged 7 commits into
numpy:masterfrom
mattip:gcc-4.8

Conversation

@mattip

@mattip mattip commented Oct 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Add a gcc 4.8 build to Azure, with the goal of properly adding a std=c99 flag

@mattip
mattip force-pushed the gcc-4.8 branch 2 times, most recently from 0ac1817 to d1c67b0 Compare October 24, 2019 15:55
mattip
mattip commented Oct 24, 2019
View reviewed changes
Comment thread numpy/distutils/system_info.py Outdated

@mattip mattip Oct 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Before this change, building locally with a broken compiler would result in the call to _c_string_literal below erroring out

  File "/home/matti/pypy_stuff/numpy/numpy/distutils/system_info.py", line 1587, in get_atlas_version
    'ATLAS_INFO', _c_string_literal(atlas_version))
  File "/home/matti/pypy_stuff/numpy/numpy/distutils/system_info.py", line 177, in _c_string_literal
    s = s.replace('\\', r'\\')
AttributeError: 'NoneType' object has no attribute 'replace'

@mattip

mattip commented Oct 24, 2019

Copy link
Copy Markdown
Member Author

It seems the place to add the std=c99 flag is in numpy/distutils/ccompiler.CCompiler_customize. This should be called only a few times, but is called ~60 times by the system_info lapack discovery phase since it does not cache the compiler. So I would like to try to compile some c99 code and see if it fails, but it seems excessive to compile it each time.

@rgommers

rgommers commented Oct 24, 2019

Copy link
Copy Markdown
Member

CCompiler_customize seems like a reasonable place to add this indeed. It should always be called (we're not enforcing that, although we should - it leads to hard to detect bugs otherwise).

So I would like to try to compile some c99 code and see if it fails, but it seems excessive to compile it each time.

Just compile it once and cache the result?

@charris

charris commented Oct 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

So I would like to try to compile some c99 code and see if it fails,

IIRC, the bit that caused problems was declarations in for loops:

for(int i = 0; i < 3; ++i);

Declaration after statement was another, but I think that passed without c99,

@mattip

mattip commented Oct 24, 2019

Copy link
Copy Markdown
Member Author

I could not find a way to quiet the compiler output when calling self.compile inside CCompiler_customize. I think adding the -std=c99 flag will always be safe when the compiler name is gcc

mattip
mattip commented Oct 25, 2019
View reviewed changes
Comment thread azure-pipelines.yml
# azure config for mac os -- Microsoft has indicated
# they will patch this issue
vmImage: macOS-10.13
vmImage: macOS-10.14

@mattip mattip Oct 25, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This change is enough to fix the macOS failure

charris
charris reviewed Oct 25, 2019
View reviewed changes
Comment thread doc/release/upcoming_changes/14771.improvement.rst Outdated
@@ -0,0 +1,6 @@
``std=c99`` added if compiler is named ``gcc``
----------------------------------------------

@charris charris Oct 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think the blank line is needed here. I edit them out of the final notes.

charris
charris reviewed Oct 25, 2019
View reviewed changes
Comment thread numpy/distutils/ccompiler.py Outdated
'clang' in self.compiler[0]):
self._auto_depends = True
if 'gcc' in self.compiler[0]:
# add std=c99 flag for gcc < 5

@charris charris Oct 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment no longer seems valid. Is the version actually checked?

@charris

charris commented Oct 25, 2019

Copy link
Copy Markdown
Member

Seems to work, just a couple of nits. Might enable travis as you touched those tests also.

@mattip

mattip commented Oct 26, 2019

Copy link
Copy Markdown
Member Author

Fixed the review comments. Travis is now running on this PR

@charris

charris commented Oct 26, 2019

Copy link
Copy Markdown
Member

Any idea what is going on with the "abandoned" failing tests?

@charris

charris commented Oct 26, 2019

Copy link
Copy Markdown
Member

I don't think the failing tests are relevant, but they also should not be failing. I'm going to close/reopen to see if that changes anything.

@charris charris closed this Oct 26, 2019
@charris charris reopened this Oct 26, 2019
@charris

charris commented Oct 26, 2019

Copy link
Copy Markdown
Member

Arrggghhhh, those staged azure tests really slow things down, the stages run serially rather than in parallel so that it takes twice as long. What a horrible waste of time :)

@charris
charris merged commit d0ce0b3 into numpy:master Oct 26, 2019
@charris

charris commented Oct 26, 2019

Copy link
Copy Markdown
Member

Thanks Matti.

@mattip mattip mentioned this pull request Nov 11, 2019
Closed
@mattip
mattip deleted the gcc-4.8 branch October 13, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charris charris charris left review comments

Assignees

No one assigned

Labels

05 - Testing component: build

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mattip @rgommers @charris