configure: use -iquote for non-system include paths #7169

hjelmn · 2019-11-14T23:58:07Z

This fixes a bug in the configuration system identified by a
change in the C++ standard. C++20 adds a new mandatory header
named version. This (and any system) header will always be
included with <> and not "". On case-insensitive filesystems
this new header conflicts with the VERSION file at the top
level of the build tree.

To fix this issue Open MPI needs to use -iquote instead of -I
for non-system include paths to ensure that these include are
only searched for the quote form of include. This commit also
adds a check to ensure that if the compiler does not support
-iquote that it falls back to -I until support can be added.

Fixes #7155

Signed-off-by: Nathan Hjelm [email protected]

hjelmn · 2019-11-14T23:59:09Z

@isuruf Please test to verify this fixes the issue.

ibm-ompi · 2019-11-15T00:08:45Z

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/413fde5d1cc19b7aef62ab7b8976026e

ibm-ompi · 2019-11-15T00:11:28Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/4e1d87832c39a6b596d5567ae6df7e3b

isuruf · 2019-11-15T00:20:29Z

Unfortunately this does not work. This would work only if there were only implementation files in the repo directory and they used quotes to find the header files.

ggouaillardet · 2019-11-15T00:34:54Z

I'll have a look at it and see if that can be fixed.

BTW, +1 for the branch name :-)

rhc54 · 2019-11-15T01:12:22Z

I strongly disagree with the WG21 person who told you this is "our problem". Their action is equivalent to us declaring a global symbol called "x" and then insisting that any program which conflicts with that symbol is "wrong".

The accepted way of "taking" a namespace is to prefix or suffix the name with something associated with your software. Thus, WG21 should have declared the required file to be something like "version.cpph" or "cpp.version.h" - and not just blanket declare ownership of the name "version".

This whole thing is absurd and unnecessary. If some standards body is going to be this stupid, then we shouldn't enable them. Just detect that the compiler has a conflict with "version" and abort with an error message directing the user to use some other compiler - and tell their vendor to stop being dumb about it.

hjelmn · 2019-11-15T01:43:45Z

@ggouaillardet I see the problem. Fixing.

hjelmn · 2019-11-15T01:46:17Z

@rhc54 I don't disagree with you in the slightest. I do think with should disallow #include <> for internal Open MPI headers so this PR is not the worst idea :).

hjelmn · 2019-11-15T01:48:43Z

@ggouaillardet @rhc54 The discussion is better in a private-ish channel. More in slack.

ggouaillardet · 2019-11-15T02:15:00Z

@hjelmn I just noted your comment and pushed my fixes to this PR

hjelmn · 2019-11-15T02:15:53Z

@ggouaillardet Indeed. Beat me by 30 seconds! Ok, I will reorder the changes now so the fixes come before the -iquote change.

hjelmn · 2019-11-15T02:17:48Z

@ggouaillardet Done. @isuruf Should work now.

ggouaillardet · 2019-11-15T02:23:03Z

@hjelmn feel free to squash both commits and/or reword my commit message

hjelmn · 2019-11-15T02:24:18Z

@ggouaillardet They should be separate I think. Makes it clear why that change was done.

ggouaillardet · 2019-11-15T02:35:42Z

fair enough, and in that case, my commit message has to be rewritten.

hjelmn · 2019-11-15T02:55:16Z

Indeed. Will update the commit message.

jsquyres · 2019-11-15T15:11:43Z

This looks like a good idea (many thanks, @hjelmn!), but I'm really not going to have an opportunity to think about this deeply / review it properly until after SC. Can we hold off on merging until after SC / Thanksgiving, perchance?

configure.ac

hjelmn · 2019-11-15T17:04:20Z

Did a little more digging:

gcc/icc/g++/icpc: -I directories will take precedence over stdc++ and system directories. must use -iquote.
xlc/xlc++: c++ standard library search paths take precedence over -I unless -qcpp_stdinc is specified. so nothing to do.
pgcc/pgc++: I don't see anything we can do here other than explicitly add a -I for the stdc++ search path. -I always takes precedence.

hjelmn · 2019-11-15T21:33:20Z

This PR will only be merged after the thanksgiving break since the next two weeks are busy for many of us.

isuruf · 2019-11-16T03:46:07Z

I tried this on OSX with clang 9 and it works. Thanks @hjelmn, @ggouaillardet

ompi/mpi/fortran/use-mpi-tkr/mpi.F90

This commit fixes an issue with the include usage in some ompi source files. These source files are using the <> form of include when the "" form is correct (as these are internal, **not** system headers). Signed-off-by: Gilles Gouaillardet <[email protected]> Signed-off-by: Nathan Hjelm <[email protected]>

This fixes a bug in the configuration system identified by a change in the C++ standard. C++20 adds a new mandatory header named version. This (and any system) header will always be included with <> and not "". On case-insensitive filesystems this new header conflicts with the VERSION file at the top level of the build tree. To fix this issue Open MPI needs to use -iquote instead of -I for non-system include paths to ensure that these include are only searched for the quote form of include. This commit also adds a check to ensure that if the compiler does not support -iquote that it falls back to -I until support can be added. References open-mpi#7155 Signed-off-by: Nathan Hjelm <[email protected]>

jsquyres · 2020-03-10T12:39:41Z

@hjelmn Somehow this PR fell off our radar in the post-SC/Thanksgiving/Christmas crush.

I just rebased it to top of master and fixed some conflicts. Please verify, and then let's merge.

jsquyres · 2020-03-12T14:23:35Z

This time, Cray had a false error (network connectivity to github). Try again...

bot:ompi:retest

hjelmn · 2020-03-30T16:15:07Z

Looks good to go.

jsquyres · 2020-03-30T16:21:16Z

@hjelmn We require github reviews on master now. Can you please give a formal review? 😄

hjelmn · 2020-03-30T16:22:05Z

Can i review my own PR?

jsquyres · 2020-03-30T16:23:01Z

ZOMG. I forgot this was your PR! Hahaha!

Ok, I just reviewed. :-)

This fixes a bug in the configuration system identified by a change in the C++ standard. C++20 adds a new mandatory header named version. This (and any system) header will always be included with <> and not "". On case-insensitive filesystems this new header conflicts with the VERSION file at the top level of the build tree. To fix this issue PMIx needs to use -iquote instead of -I for non-system include paths to ensure that these include are only searched for the quote form of include. This commit also adds a check to ensure that if the compiler does not support -iquote that it falls back to -I until support can be added. Tracks open-mpi/ompi#7169 Signed-off-by: Ralph Castain <[email protected]>

This fixes a bug in the configuration system identified by a change in the C++ standard. C++20 adds a new mandatory header named version. This (and any system) header will always be included with <> and not "". On case-insensitive filesystems this new header conflicts with the VERSION file at the top level of the build tree. To fix this issue PRRTE needs to use -iquote instead of -I for non-system include paths to ensure that these include are only searched for the quote form of include. This commit also adds a check to ensure that if the compiler does not support -iquote that it falls back to -I until support can be added. Tracks open-mpi/ompi#7169 Fixes openpmix#481 Signed-off-by: Ralph Castain <[email protected]>

This fixes a bug in the configuration system identified by a change in the C++ standard. C++20 adds a new mandatory header named version. This (and any system) header will always be included with <> and not "". On case-insensitive filesystems this new header conflicts with the VERSION file at the top level of the build tree. To fix this issue PMIx needs to use -iquote instead of -I for non-system include paths to ensure that these include are only searched for the quote form of include. This commit also adds a check to ensure that if the compiler does not support -iquote that it falls back to -I until support can be added. Tracks open-mpi/ompi#7169 Signed-off-by: Ralph Castain <[email protected]>

hjelmn mentioned this pull request Nov 14, 2019

Rename VERSION to VERSION.txt #7163

Closed

hjelmn force-pushed the fix_what_wg21_calls_our_problem_not_theirs_seriously__in_some_ways_they_are_correct_but_wtf branch from 1b3111f to a048b3c Compare November 15, 2019 02:17

ggouaillardet force-pushed the fix_what_wg21_calls_our_problem_not_theirs_seriously__in_some_ways_they_are_correct_but_wtf branch from a048b3c to a393f68 Compare November 15, 2019 04:08

hjelmn force-pushed the fix_what_wg21_calls_our_problem_not_theirs_seriously__in_some_ways_they_are_correct_but_wtf branch from a393f68 to 4237a6c Compare November 15, 2019 04:27

ggouaillardet force-pushed the fix_what_wg21_calls_our_problem_not_theirs_seriously__in_some_ways_they_are_correct_but_wtf branch from 4237a6c to 194f681 Compare November 15, 2019 04:54

bwbarrett reviewed Nov 15, 2019

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

hjelmn force-pushed the fix_what_wg21_calls_our_problem_not_theirs_seriously__in_some_ways_they_are_correct_but_wtf branch 2 times, most recently from a8b40bb to 6ea574b Compare November 15, 2019 19:41

hjelmn added bug Target: main ⚠️ WIP-DNM! labels Nov 15, 2019

jsquyres reviewed Nov 16, 2019

View reviewed changes

ompi/mpi/fortran/use-mpi-tkr/mpi.F90 Show resolved Hide resolved

minrk mentioned this pull request Dec 5, 2019

Remove C++ bindings conda-forge/openmpi-feedstock#53

Merged

ggouaillardet and others added 2 commits March 9, 2020 21:13

jsquyres force-pushed the fix_what_wg21_calls_our_problem_not_theirs_seriously__in_some_ways_they_are_correct_but_wtf branch from 6ea574b to a6212fe Compare March 10, 2020 12:38

jsquyres removed the ⚠️ WIP-DNM! label Mar 30, 2020

jsquyres added NEWS Target: v5.0.x labels Mar 30, 2020

jsquyres added this to the v5.0.0 milestone Mar 30, 2020

jsquyres approved these changes Mar 30, 2020

View reviewed changes

hjelmn merged commit 160ff18 into open-mpi:master Mar 30, 2020

This was referenced Mar 30, 2020

Configure: use -iquote for non-standard paths openpmix/prrte#481

Closed

Configure: use -iquote for non-standard paths openpmix/openpmix#1691

Closed

rhc54 mentioned this pull request Apr 4, 2020

configure: use -iquote for non-system include paths openpmix/openpmix#1710

Merged

rhc54 mentioned this pull request Apr 6, 2020

configure: use -iquote for non-system include paths openpmix/prrte#500

Merged

jeffhammond mentioned this pull request Dec 1, 2021

nvfortran-Error-Unknown switch: -iquote../../../../../.. #9716

Closed

Uh oh!

configure: use -iquote for non-system include paths #7169

configure: use -iquote for non-system include paths #7169

Uh oh!

Conversation

hjelmn commented Nov 14, 2019 • edited by jsquyres Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hjelmn commented Nov 14, 2019

Uh oh!

ibm-ompi commented Nov 15, 2019

Uh oh!

ibm-ompi commented Nov 15, 2019

Uh oh!

isuruf commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggouaillardet commented Nov 15, 2019

Uh oh!

rhc54 commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

ggouaillardet commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

ggouaillardet commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

ggouaillardet commented Nov 15, 2019

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

jsquyres commented Nov 15, 2019

Uh oh!

Uh oh!

hjelmn commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hjelmn commented Nov 15, 2019

Uh oh!

isuruf commented Nov 16, 2019

Uh oh!

Uh oh!

jsquyres commented Mar 10, 2020

Uh oh!

jsquyres commented Mar 12, 2020

Uh oh!

hjelmn commented Mar 30, 2020

Uh oh!

jsquyres commented Mar 30, 2020

Uh oh!

hjelmn commented Mar 30, 2020

Uh oh!

jsquyres commented Mar 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hjelmn commented Nov 14, 2019 •

edited by jsquyres

Loading

isuruf commented Nov 15, 2019 •

edited

Loading

hjelmn commented Nov 15, 2019 •

edited

Loading