Rename VERSION to VERSION.txt

[isuruf]

libc++ v8.0.0 and above uses a header named in the C++
standard headers. VERSION file in this repo root conflicts with the
header from libc++ on case-insensitive file systems when building
the C++ bindings because the repo root is added to the search
directories by the build system.

Fixes #7155

[ompiteam-bot]

Can one of the admins verify this patch?

[bwbarrett]

ok to test

[rhc54]

I can't speak for everyone, but I really don't agree with this change. This is a problem outside of our scope, and this solution just begs for yet another conflict with some other package that uses VERSION.txt.

Let's not chase our tails.

[ibm-ompi]

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

Gist: https://gist.github.com/2d2e5fa4f036dd2321fa27cc5dd4bb22

[ibm-ompi]

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

Gist: https://gist.github.com/e9031b2a81e271b01f3cbbca75ae3369

[bwbarrett]

I can't speak for everyone, but I really don't agree with this change. This is a problem outside of our scope, and this solution just begs for yet another conflict with some other package that uses VERSION.txt.

Let's not chase our tails.

I don't completely disagree, but it's also part of the C++ standard (as of C++20). So this will be a problem any time a C++ compiler is used to compile Open MPI; it's not a bug in the compiler or the standard library; they are required to provide that file. Of course, that file is supposed to be named version and MacOS should have cut out the silliness with case-insensitive filesystems a decade ago, but here we are. There's no fix for this issue that isn't work for Open MPI. I think the chances of the C or C++ committees requiring a header named VERSION.txt is lower, so at least there's that.

All that being said, please do not merge this patch in at this time. I'm worried about fall-out impacts on build / test infrastructure. We should think about going down this option, but 1) I know there are more places than configure.ac that look at the VERSION file, some of which are outside of the Open MPI tarball and 2) with SC next week, there's very little time to clean that mess up. Maybe during Thanksgiving (US) week while things are calm?

[rhc54]

Agreed. Let's talk about it on one of the calls. My concern is that we will just wind up repeatedly renaming files as we keep hitting such issues. Only real solution I can see is to prefix/suffix the filenames with something OMPI specific, and doing that for every file with a commonly used name (e.g., README, INSTALL, HACKING, etc) for consistency.

Other option that seems really easy: just tell people not to use C++20 compilers to build OMPI - there is no valid reason for doing so anyway.

I personally think that any standard that requires a file named "version" be unique in the system has violated all common sense. That filename is routinely used by everyone, not just OMPI. I'm sure they are getting an earful about it from across the community.

[jsquyres]

@isuruf You might want to also try make distcheck and contrib/dist/make_dist_tarball. I don't remember offhand of those will read VERSION somehow...

Brian is right: that VERSION file is read all over the place. The next two weeks unfortunately kinda suck in terms of timing. 😢

[ggouaillardet]

An other option is to create a src directory and move all source trees there, so we do not need to -I${top_srcdir} any more (this is where VERSION is) but -I${top_srcdir}/src instead.

Of course, we can always get rid of the C++ bindings and the need for a C++ compiler will magically go away.

[rhc54]

Of course, we can always get rid of the C++ bindings and the need for a C++ compiler will magically go away.

+10!

[rhc54]

Seriously, we should get rid of them for OMPI v5 at the very least - they were removed from the standard quite a while ago.

[ggouaillardet]

I pushed two more commits to get the fix pass the tests.
@isuruf feel free to squash them into your initial commit.

Strictly speaking, the issue is not only triggered by a C++ compiler used to build Open MPI.
The issue occurs when the C++ compiler builds the C++ bindings that use headers from libc++ that end up pulling headers that #include <version>.
An other way to put it is that we can build Open MPI with a C++ compiler and libc++ as long as we do not build the C++ MPI bindings.

[jsquyres]

Actually, that's a good way of thinking about this: take a step back and think about other ways to solve this issue.

We/Open MPI are being a bit "odd" in that we -I our top-level source directory. I never would have thought that this would be a problem, but here we are.

That being said, I wonder if -I$(top_srcdir) is necessary when compiling the C++ bindings. We don't need to do anything as drastic as moving our entire source trees down to src/ -- but compiling the C++ bindings is small, segregated thing. Perhaps there's a way in there to not -I the top-level source directory...? (I have not investigated this possibility at all)

Given that this horribly-named <version> header file is in C++20, don't forget that even if we remove the C++ bindings in Open MPI v5, we still should do something about this for v4.0.x (and possibly even v3.0.x/v3.1.x).

FWIW, in https://github.com/open-mpi/ompi/wiki/5.0.x-FeatureList: "...we all seem to agree that removing the C++ bindings in v5.0.x is not a problem."

[ggouaillardet]

I already tried without -I${top_srcdir} and it does not work. Better move the sources into a new src directory imho.

[bosilca]

As @rhc54 suggested above why not addressing this particular issue by preventing the use of any C++ compiler subject to the name collision issue ? We can easily detect this during configure, stop the build and dump an error message suggesting the use of a different compiler.

[bwbarrett]

As @rhc54 suggested above why not addressing this particular issue by preventing the use of any C++ compiler subject to the name collision issue ? We can easily detect this during configure, stop the build and dump an error message suggesting the use of a different compiler.

@bosilca the file is mandated in C++20, so the number of compilers with this issue will go up, not down. Probably not a good plan.

[hjelmn]

@bwbarrett Wait, so WG21 is responsible for this mess? I have to wonder WTH they were thinking?

[isuruf]

FYI, it's http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0754r2.pdf

[hjelmn]

@isuruf Thank you for adding that.

So, wouldn't it be better to do this for the releases only and then in master just run git rm -r ompi/mpi/cxx and be done with it.

[gpaulsen]

I like the git rm -r ompi/mpi/cxx idea, but we should probably do it in master ALSO in case anyone has non-opensourced ompi components written in C++... :)

[hjelmn]

@gpaulsen True. Didn't think of that case. Short of somehow getting rid of -I${top_srcdir} this might be the only way to deal with WG21's odd decision.

[rhc54]

I suspect WG21 is going to have to re-address this issue. Imagine all of the packages out there that have a "VERSION" file in them - does anyone actually believe that every one of them is going to change their file name just to accommodate an idiotic C++ decision??

Let's not overreact and do something right away - let's give this a bit to see what happens long term. For now, just put in a check and abort if someone uses a bad compiler.

[hjelmn]

@rhc54 I have a friend who participates in WG21 meetings. I will ping him and see if I can find out more and communicate the issue. There is likely still time to change C++20 before it is ratified.

[bosilca]

@bwbarrett the number of CXX+20 compilers will hopefully go up, but the number of people compiling OMPI with a CXX compiler on a case-insensitive file system will most certainly not follow the same trend.

We can waste hours trying to find a better solution for such a minor issue (in number of potentially impacted users), or take the pragmatic approach and not support this combination of OS and CXX compilers.

[isuruf]

@rhc54 I have a friend who participates in WG21 meetings. I will ping him and see if I can find out more and communicate the issue.

Thanks @hjelmn. FWIW, this is the 3rd time I'm running into this issue in the last couple of weeks (chromium, igraph, openmpi) after updating the compiler.

[jsquyres]

Thanks @hjelmn. FWIW, this is the 3rd time I'm running into this issue in the last couple of weeks (chromium, igraph, openmpi) after updating the compiler.

That's a very valuable data point.

I was thinking that we were pretty unusual (i.e., adding -IDIR where DIR has a VERSION file in it, because DIR in our case is our top-level directory). But you're saying you've run into this same issue in 2 other projects? Huh. 😱

Please remember what I said above:

• We're potentially going to delete the C++ bindings in Open MPI v5.0.x (it's on the wiki / up for consideration in v5.0, at least). So yes, the problem may solve itself for master/v5.0.x. Great!
• But this doesn't fix the problem for v4.0.x (and/or v3.1.x and/or v3.1.x).

So ya -- let's think about this and come up with a palatable solution. It's a stupid issue, but it's unfortunately one that we should probably solve (although we have time -- per above, the timing sucks, and we can spend a little time thinking about this). My $0.02 is that renaming VERSION --> VERSION.txt vs. moving the project dirs under src are going to be equal-ish in terms of effort.

I'm not concerned about compilers someday including README, INSTALL, HACKING, or other files.

[hjelmn]

Got a response. This is not the correct fix. PR coming shortly.

[hjelmn]

Replaced by #7169