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

Using C++17 #17

Closed
jakirkham opened this issue May 20, 2020 · 62 comments · Fixed by #75
Closed

Using C++17 #17

jakirkham opened this issue May 20, 2020 · 62 comments · Fixed by #75

Comments

@jakirkham
Copy link
Member

It looks like we are currently requiring C++14 on macOS; whereas, we use C++17 on Linux. Would it be possible to start using C++17 on macOS as well?

@chrisburr
Copy link
Member

I thought we were doing this already 😄

@conda-forge/clang-compiler-activation Are there any objections to changing macOS to use C++17 by default or should I prepare the PR?

@jakirkham
Copy link
Member Author

I'd take silence here as no objections Chris. Do you want to submit a PR? 🙂

@isuruf
Copy link
Member

isuruf commented Mar 5, 2021

This needs to be co-ordinated with a boost update as changing the C++ standard changes boost ABI.

@jakirkham
Copy link
Member Author

So we would piggy back on a Boost update and subsequent pinning migrator? That makes sense. Shouldn't be too difficult then. Are there any other issues you foresee?

@jakirkham
Copy link
Member Author

Looking at past Boost version history, it seems like a new release is probably due around April.

@isuruf
Copy link
Member

isuruf commented May 4, 2021

I don't think we should update the C++ version and remove the -std=c++14 flag altogether from CXXFLAGS env variable.

@jakirkham
Copy link
Member Author

Went ahead and put this on the agenda for tomorrow so we can figure out how we want to handle this

@xhochy
Copy link
Member

xhochy commented May 21, 2021

@jakirkham @isuruf What was the outcome of the core meeting on this?

@chrisburr
Copy link
Member

We suspect the boost ABI doesn't change between C++ versions (except pre/post C++11) but someone needs to build boost with both versions and diff the available symbols in the shared libraries to check.

@jakirkham
Copy link
Member Author

If that's true, we are planning to drop -std=c++14 from CXXFLAGS as Isuru noted above

@xhochy
Copy link
Member

xhochy commented May 21, 2021

It actually does change!

I've built boost-cpp once with -std=c++14 and once with -std=c++17 and it revealed the following ABI differences:

Script:

from pathlib import Path
from subprocess import check_output

for lib_14 in list(Path("cxx14/lib").glob("*.dylib")):
    lib_17 = Path("cxx17") / "lib" / lib_14.name
    exports_14 = {line.split(b" ")[2] for line in check_output(f"nm {lib_14}", shell=True).split(b"\n") if b" " in line}
    exports_17 = {line.split(b" ")[2] for line in check_output(f"nm {lib_17}", shell=True).split(b"\n") if b" " in line}
    if exports_14 != exports_17:
        print(lib_14.name)
        print("-" * len(lib_14.name))
        print(exports_14 - exports_17)
        print(exports_17 - exports_14)
        print()

Output:

libboost_locale.dylib
---------------------
{b'__ZN5boost6locale4util23create_simple_converterERKNSt3__112basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEE', b'__ZN5boost6locale4util21create_utf8_converterEv', b'__ZN5boost6locale28localization_backend_manager11add_backendERKNSt3__112basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEENS2_8auto_ptrINS0_20localization_backendEEE', b'__ZN5boost6locale4util14create_codecvtERKNSt3__16localeENS2_8auto_ptrINS1_14base_converterEEEj', b'__ZNK5boost6locale28localization_backend_manager3getEv'}
set()

libboost_context.dylib
----------------------
{b'__ZNSt3__117__call_once_proxyINS_5tupleIJRFvP6rlimitEOS3_EEEEEvPv', b'__ZNSt3__117__call_once_proxyINS_5tupleIJRFvPmEOS2_EEEEEvPv'}
{b'__ZNSt3__117__call_once_proxyINS_5tupleIJRDoFvP6rlimitEOS3_EEEEEvPv', b'__ZNSt3__117__call_once_proxyINS_5tupleIJRDoFvPmEOS2_EEEEEvPv'}

@jakirkham
Copy link
Member Author

That's unfortunate. Probably no way to know this, but are those symbols actually used externally or are they just used by Boost?

@xhochy
Copy link
Member

xhochy commented Sep 14, 2021

This stalled for quite some while but we increasingly see packages that require e.g. a C++17 enabled build of abseil-cpp. If someone can list todos, then I would look a bit into doing this; I'm currently out of ideas and would default to just remove std=c++14 from the flags and then see what breaks.

@isuruf
Copy link
Member

isuruf commented Sep 14, 2021

Can you run your script above with the latest version?

@BastianZim
Copy link
Member

BastianZim commented Oct 19, 2021

@xhochy Would you mind running your script again? As I now have or-tools (Where we first talked about this) almost ready and would like to start the OSX version.

@jakirkham
Copy link
Member Author

Given the code is there, this is something you could run if it is of interest @BastianZim :)

@BastianZim
Copy link
Member

BastianZim commented Oct 19, 2021

Bit above my abilities, I'm afraid... Otherwise, I would've. :)

Edit: Or do you mean just adding the respective flags to https://github.com/conda-forge/boost-feedstock/blob/master/recipe/build.sh and running build-locally.py?

Edit2: Based on further researching I assume you don't. 😄
I mean I'm happy to look into this because you guys are probably already swamped with cf stuff as it is but I would need some pointers...

@jakirkham
Copy link
Member Author

Meaning this ( #17 (comment) )

Yeah can build with each flag locally and then compare :)

@BastianZim
Copy link
Member

BastianZim commented Oct 19, 2021

Yes the python part is understood it was about the boost part. Wasn't sure if that includes building the conda-forge distribution or the upstream version?

Ok bit late here but will report back tomorrow.

@BastianZim
Copy link
Member

@jakirkham
Ok am now more awake. :)
My understanding is that I first need to change this feedstock and then use this feedstock to build boost, is that incorrect? That's why I had initially assumed that it would be too difficult for me.

If I can just set the flags when building the boost feedstock and they are respected I can definitely do that.

@jakirkham
Copy link
Member Author

Great! 😄

I don't think we need to change this feedstock per se (though others should feel free to correct me). As long as we are able to build Boost with C++14 and C++17 by just configuring the Boost build differently, that should be sufficient

@BastianZim
Copy link
Member

I think I really am out of my depth here, sorry. I built both but the only cxx14 and cxx17 folders I can find are under boost-feedstock/build_artifacts/src_cache/boost_1_76_0/boost/algorithm which I doubt are the ones you're talking about.

@jakirkham
Copy link
Member Author

This was boost-cpp? Or boost? I think we want the former as the latter is mainly just Boost.Python

The packages should show up in the build_artifacts directory in the feedstock (assuming the feedstock's build script is used)

@BastianZim
Copy link
Member

BastianZim commented Oct 20, 2021

🤦‍♂️ Looking in the right folder actually helps, who knew? 😄

Small problem though because I'm unable to build with OSX, only Linux works. The former fails with MacOSX10.9.sdk/usr/share/man/man8/setregion.8: Failed to create dir 'MacOSX10.9.sdk' and I currently don't have a different machine to access and try.

@jakirkham
Copy link
Member Author

Yeah don't think that is documented unfortunately. Filed an issue ( conda-forge/conda-forge.github.io#1530 )

Checking on Linux is probably good enough to start (others should feel free to correct me)

@BastianZim
Copy link
Member

No I did look in build_artifacts just not everywhere, my bad. Sorry for the confusion.

Checking on Linux is probably good enough to start (others should feel free to correct me)

Are you sure? Because I thought you'd be interested in whenever it changes on OSX. Linux should also have changes, I would assume, but I'm not sure if they're the same as on OSX.

@BastianZim
Copy link
Member

And just so that we're on the same page: You're talking about building boost but adding the respective flags to https://github.com/conda-forge/boost-cpp-feedstock/blob/94018f490328c05d805e4f030e4a1830b33e5478/recipe/build.sh#L57-L73 correct?

@jakirkham
Copy link
Member Author

That's right :)

@BastianZim
Copy link
Member

BastianZim commented Oct 21, 2021

Worked!

I cannot detect any differences between the two versions using the script above. Furthermore, libboost_locale.so and libboost_context.so are the same according to cmp --silent file1 file2 && echo '### SUCCESS: Files Are Identical! ###' || echo '### WARNING: Files Are Different! ###'.

The packages are at https://anaconda.org/bastianzim/boost-cpp (build 0 is 14 and build 1 is 17).

@BastianZim
Copy link
Member

Gentle ping here @jakirkham

@BastianZim
Copy link
Member

@jakirkham Any news?

@jakirkham
Copy link
Member Author

Sorry not following. What's the question?

@BastianZim
Copy link
Member

My findings in #17 (comment) based on what we had discussed above.

@jakirkham
Copy link
Member Author

jakirkham commented Oct 29, 2021

Right one thing I think we should check (and was meaning to do this, but haven't had a chance) is verify the libraries built used the C++ version we expect (IOW Boost didn't disregard compile flags we gave it). That said, it may not obvious from rudimentary inspection. Though it may be possible to record the flags in the libraries and verify that way.

Edit: Isuru may have better ideas here

@jakirkham
Copy link
Member Author

Another way to approach the problem above would be to look for any symbols that Boost includes with specific C++ versions. For example std::invoke would be one of these on C++17. It should be absent in C++14

@BastianZim
Copy link
Member

boost/context/detail/invoke.hpp is unfortunately available in both.

@jakirkham
Copy link
Member Author

Think it just copies all the headers no matter what. So don't think that would change based on C++ standard used

What we would want to check is nm on a relevant library (perhaps like the one above) to see if they are getting C++17 symbols in that case

If we can confirm that, this should help give us some confidence that Boost built how we expected and indeed the ABI/API surface hasn't changed between C++ standards

@BastianZim
Copy link
Member

BastianZim commented Oct 31, 2021

Ok, so diff -qr /boost/build_artifacts_14/linux-64/boost-cpp-1.77.0-h312852a_0/include/boost /boost/build_artifacts_17/linux-64/boost-cpp-1.77.0-h312852a_1/include/boost gave no results. I'll try rebuilding it with -frecord-gcc-switches based on your comment.

Edit: Quick question, where should that flag be added because if I add it to the flags it fails.

@jakirkham
Copy link
Member Author

Would try CFLAGS and CXXFLAGS. Or is that what you tried already?

@BastianZim
Copy link
Member

BastianZim commented Nov 1, 2021

Ahh CXXFLAGS worked, thanks.

Using strings I get the following:

strings /boost-cpp-feedstock/build_artifacts/linux-64/boost-cpp-1.77.0-h312852a_1/lib/libboost_chrono.so|grep "c++17"     
-std=c++17

c++14 (In the -std=c++17 package) gives me nothing.

Do note though that I'm using a Mac here to read Linux files, not sure if something gets mangled along the way. 😄

@BastianZim
Copy link
Member

@jakirkham Anything else that should be checked?

@jakirkham
Copy link
Member Author

Not atm. @isuruf @xhochy do you have any more thoughts/questions/concerns here?

@BastianZim
Copy link
Member

Just a super gentle ping here, as I'd need this for a feedstock - thanks!

@isuruf
Copy link
Member

isuruf commented Nov 15, 2021

? Individual feedstocks can use C++17 if they want

@BastianZim
Copy link
Member

I need it for or-tools which depends on abseil-cpp where I was directed to this issue by @xhochy.

Ref: conda-forge/staged-recipes#16147 (comment)

@isuruf
Copy link
Member

isuruf commented Nov 15, 2021

Why does or-tools and abseil-cpp need to have the same C++ language version?

@xhochy
Copy link
Member

xhochy commented Nov 15, 2021

abseil-cpp does have a varying API based on whether it was compiled with C++17 or not (mostly around string views)

@isuruf
Copy link
Member

isuruf commented Nov 15, 2021

So, that can be changed only when there's a new abseil-cpp version I guess?

@xhochy
Copy link
Member

xhochy commented Nov 15, 2021

Yes, there is a release outstanding: conda-forge/abseil-cpp-feedstock#24

@h-vetinari
Copy link
Member

Not sure where's the best place to discuss windows in relation to this, but I opened an issue on the vc-feedstock: conda-forge/vc-feedstock#45

@h-vetinari
Copy link
Member

OK, since the conclusion of this thread wasn't clear (to me), I repeat what @jakirkham noted in the vc-issue:

At least with macOS and Linux, we dropped these flags ( conda-forge/clang-compiler-activation-feedstock#75 ) to leave it up to recipes.

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

Successfully merging a pull request may close this issue.

6 participants