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

Fix cugraph_c target name in Python builds #3045

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Dec 6, 2022

pylibcugraph is currently linking to the cugraph_c target directly rather than via the exported alias cugraph::cugraph_c. I am unsure under exactly what circumstances the former should be working, but I don't think it is reliable. I see failures locally without the change. I have currently made this PR into 22.12 because it seems like an important fix, but I can push it to 23.02 if needed.

@vyasr vyasr added bug Something isn't working 3 - Ready for Review non-breaking Non-breaking change labels Dec 6, 2022
@vyasr vyasr requested a review from a team as a code owner December 6, 2022 01:21
@vyasr vyasr self-assigned this Dec 6, 2022
@vyasr vyasr changed the base branch from branch-23.02 to branch-22.12 December 6, 2022 01:21
@vyasr
Copy link
Contributor Author

vyasr commented Dec 6, 2022

I'd like @robertmaynard's take on whether this is critical to get into 22.12. I'm surprised that this isn't breaking more builds right now since I can reproduce failures without it locally (it won't affect wheel builds, or any other build where the C++ library is built as part of the Python package, but I would expect it to break conda builds). I am curious if the issue in #3041 was just blocking builds from getting far enough to notice this issue.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.12@0502585). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.12    #3045   +/-   ##
===============================================
  Coverage                ?   58.80%           
===============================================
  Files                   ?      131           
  Lines                   ?     7836           
  Branches                ?        0           
===============================================
  Hits                    ?     4608           
  Misses                  ?     3228           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@robertmaynard
Copy link
Contributor

I'd like @robertmaynard's take on whether this is critical to get into 22.12.

I would say this is critical only if it is blocking getting conda builds out the door. For people building from source we can provide a minimal set of instructions to unblock when building 22.12 locally.

I expect you are correct that #3041 was hiding this issue

@raydouglass raydouglass merged commit 11a7660 into rapidsai:branch-22.12 Dec 6, 2022
@vyasr vyasr deleted the fix/cugraph_c_linkage branch December 6, 2022 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants