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

Explicitly request CMake use gnu++17 over c++17 #10297

Conversation

robertmaynard
Copy link
Contributor

Currently cudf requires compiler extensions to use __int128_t with standard library containers. Ensure the correct compilation flags are always providied.

@robertmaynard robertmaynard added the CMake CMake build issue label Feb 15, 2022
@robertmaynard robertmaynard requested review from a team as code owners February 15, 2022 16:18
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 15, 2022
@robertmaynard robertmaynard added the non-breaking Non-breaking change label Feb 15, 2022
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks good. Would a comment be helpful, to indicate that extensions are only needed for 128 bit integer support?

@jjacobelli jjacobelli added the improvement Improvement / enhancement to an existing function label Feb 15, 2022
@robertmaynard
Copy link
Contributor Author

Looks good. Would a comment be helpful, to indicate that extensions are only needed for 128 bit integer support?

Good idea, done.

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #10297 (5a72c87) into branch-22.04 (8b0737d) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10297      +/-   ##
================================================
- Coverage         10.67%   10.67%   -0.01%     
================================================
  Files               122      122              
  Lines             20874    20880       +6     
================================================
  Hits               2228     2228              
- Misses            18646    18652       +6     
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 0.00% <0.00%> (ø)
python/cudf/cudf/testing/_utils.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b0737d...5a72c87. Read the comment docs.

@vyasr
Copy link
Contributor

vyasr commented Feb 15, 2022

@robertmaynard do we anticipate support for this in libcudacxx at some known point in the future? Should we make a note somewhere that we would like to switch to relying on that when we can?

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I'm going to approve this in case it's blocking builds for the ops team, but if we plan to switch to relying on libcudacxx for int128 support at some point I would like that to be noted in the CMake so that we don't lose track of that.

@robertmaynard
Copy link
Contributor Author

@robertmaynard do we anticipate support for this in libcudacxx at some known point in the future? Should we make a note somewhere that we would like to switch to relying on that when we can?

@vyasr Do you think the new comment is sufficient?

@vyasr
Copy link
Contributor

vyasr commented Feb 15, 2022

Yup that looks good to me. I'm not aware of this being a high priority on the short term cuda::std roadmap so I don't think we can say anything more specific right now.

@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 9eb6a66 into rapidsai:branch-22.04 Feb 16, 2022
@robertmaynard robertmaynard deleted the bug/explicitly_request_compiler_extensions branch February 16, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants