-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Comments
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? |
I'd take silence here as no objections Chris. Do you want to submit a PR? 🙂 |
This needs to be co-ordinated with a boost update as changing the C++ standard changes boost ABI. |
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? |
Looking at past Boost version history, it seems like a new release is probably due around April. |
I don't think we should update the C++ version and remove the |
Went ahead and put this on the agenda for tomorrow so we can figure out how we want to handle this |
@jakirkham @isuruf What was the outcome of the core meeting on this? |
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. |
If that's true, we are planning to drop |
It actually does change! I've built 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:
|
That's unfortunate. Probably no way to know this, but are those symbols actually used externally or are they just used by Boost? |
This stalled for quite some while but we increasingly see packages that require e.g. a C++17 enabled build of |
Can you run your script above with the latest version? |
@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. |
Given the code is there, this is something you could run if it is of interest @BastianZim :) |
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 Edit2: Based on further researching I assume you don't. 😄 |
Meaning this ( #17 (comment) ) Yeah can build with each flag locally and then compare :) |
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. |
@jakirkham If I can just set the flags when building the boost feedstock and they are respected I can definitely do that. |
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 |
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. |
This was The packages should show up in the |
🤦♂️ 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 |
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) |
No I did look in
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. |
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? |
That's right :) |
Worked! I cannot detect any differences between the two versions using the script above. Furthermore, The packages are at https://anaconda.org/bastianzim/boost-cpp (build 0 is 14 and build 1 is 17). |
Gentle ping here @jakirkham |
@jakirkham Any news? |
Sorry not following. What's the question? |
My findings in #17 (comment) based on what we had discussed above. |
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 |
Another way to approach the problem above would be to look for any symbols that Boost includes with specific C++ versions. For example |
|
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 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 |
Ok, so Edit: Quick question, where should that flag be added because if I add it to the flags it fails. |
Would try |
Ahh Using 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
Do note though that I'm using a Mac here to read Linux files, not sure if something gets mangled along the way. 😄 |
@jakirkham Anything else that should be checked? |
Just a super gentle ping here, as I'd need this for a feedstock - thanks! |
? Individual feedstocks can use C++17 if they want |
I need it for |
Why does |
|
So, that can be changed only when there's a new |
Yes, there is a release outstanding: conda-forge/abseil-cpp-feedstock#24 |
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 |
OK, since the conclusion of this thread wasn't clear (to me), I repeat what @jakirkham noted in the vc-issue:
|
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?
The text was updated successfully, but these errors were encountered: