-
-
Notifications
You must be signed in to change notification settings - Fork 11
Switch to building with igraph dependency #98
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
Switch to building with igraph dependency #98
Conversation
…dependencies use a compatible C core.
|
@conda-forge-admin, please rerender |
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Remove the CMake call and instead point to external `igraph`
0b6455b to
8ba3854
Compare
|
@conda-forge-admin, please rerender |
ed3aedb to
8287dc7
Compare
|
@conda-forge-admin, please rerender |
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipe/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/18059274125. Examine the logs at this URL for more detail. |
76286f6 to
5c3d6cf
Compare
|
@conda-forge-admin, please rerender |
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Added igraph_version variable to specify igraph version.
|
Thanks @xylar! Are you sure that it actually builds against the external dependency on Windows? I remember from looking at this in igraph/python-igraph#790 that on Windows it would not actually compile against the external library, but still rely on the vendored library. Maybe something changed in the meantime, so that it actually does compile correctly, also on Windows. But to be sure, I would need to go through the |
|
@vtraag, shoot, I forgot that was still a sticking point. I think you are right that Windows is still using the vendored version, not the shared one. |
|
I won't have time to work on this today. I'll put it on my to-do list for tomorrow. |
|
I am not 100% sure whether it does, but it would be good to double check indeed. I'll also look into it. I agree that this is a preferable route, and even if igraph/python-igraph#790 does not get merged, I think it makes sense to start linking to |
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipe/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/18468019582. Examine the logs at this URL for more detail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Copilot helped me to get this patched up. Have to see if all the tests pass now. Some Win tests are...
|
@vtraag, I think I have things working finally. Do you care to take another look? I added the |
|
Thanks @xylar! I'm not yet 100% convinced myself. I'd prefer to check it on a Windows machine myself, but I need to set it up. The particularly worrisome part is here in the
Can you perhaps advice further @ntamas? |
|
@vtraag, another option is to continue vendoring igraph for windows but not for Unix. That's at least a step in the right direction. We don't seem to have a clear alternative if you're not convinced by my patch. |
|
Now that we're embarking on this path, I would prefer to make the change to using the external |
|
Sounds good. |
|
We can put this in draft mode and just use it as a point of reference when the time comes. |
|
I'm not familar with |
|
Thanks @ntamas, it is reassuring that you haven't identified any other problems. I've identified the problem with the transitive dependencies. For some reason, However, for some reason, I'm unable to build locally on my Windows machine, and it keeps complaining about a missing |
|
I've now also figured out the missing |
|
I've also read up on linking to DLLs on Windows, apparently |
|
There is a tangentially related issue on It seems that The output of Which is what it should be. It might be simpler and cleaner to patch In addition, the library names in Let me know what you think @xylar. |
|
On Linux we are easily able to set an alias for |
|
@vtraag, sounds promising. I don't know when I'll have time to help out with this again but please give this a try! |
|
I've just pushed a commit to use |
|
As far as I'm concerned I'm happy with how it is built now. I've also checked things locally on my Windows machine, and that also all checks out. If you agree @xylar, then let's get this merged. |
|
@conda-forge-admin, please rerender |
|
@vtraag, looks good to me! I'm going to rerender just in case and then set this to automerge. |
|
Hi! This is the friendly conda-forge automerge bot! I considered the following status checks when analyzing this PR:
Thus the PR was passing and merged! Have a great day! |
|
I'm going to assume the test failure was one of these rare but persistent flukes. |
Checklist
conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)fixes #88
closes #97