search

conda-forge / python-graphviz-feedstock

A conda-smithy repository for python-graphviz.
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

Add a patch to make calls to Graphviz binaries work on Windows #2

Closed ccordoba12 closed 7 years ago

ccordoba12 commented 7 years ago

This is needed because Graphviz binaries (e.g. dot) are redefined as batch scripts in the Graphviz conda-forge package. Hence, it doesn't make sense to submit this patch upstream.

conda-forge-linter commented 7 years ago

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 (recipe) and found it was in an excellent condition.

jakirkham commented 7 years ago

Feel free to add yourself as a maintainer as well, @ccordoba12.

ccordoba12 commented 7 years ago

Feel free to add yourself as a maintainer as well, @ccordoba12.

Ok

jakirkham commented 7 years ago

Might also be worthwhile to re-render.

jakirkham commented 7 years ago

Is there a good way to test this change?

ccordoba12 commented 7 years ago

I'll add a test and re-render in a couple of days. I don't have more time to work on this right now :-)

jakirkham commented 7 years ago

That's fine. I'll take on the re-render. Just think about a test when you have some time.

jakirkham commented 7 years ago

Was about to upgrade to 0.6 in PR ( https://github.com/conda-forge/python-graphviz-feedstock/pull/4 ), but noticed this PR was still here. Broke out a re-rendering in PR ( https://github.com/conda-forge/python-graphviz-feedstock/pull/5 ), which has been merged. Also have rebased the 0.6 release PR so it is ready to go. Not sure if we still want to finish this up first or if we should go ahead with the 0.6 release and follow up on this afterwards. Please let me know what you think, @ccordoba12.

ccordoba12 commented 7 years ago

@jakirkham, I added a test based on dask because @mrocklin provided me with it. I really don't know how to use python-graphviz and don't have time to come up with a simpler test, so I hope you're fine with it.

Besides, one of the main users of python-graphviz among the scientific stack is dask, so I think it's good to use it as a test here.

ccordoba12 commented 7 years ago

Everything is green now, so I think this should be merged.

jakirkham commented 7 years ago

I have no qualms with the test, @ccordoba12. Though it would be nice if @xflr6 took a look (particularly at the patch).

ccordoba12 commented 7 years ago

Yep, I saw he did another PR but it's failing. I mentioned there the reasons why I think that's happening.

ccordoba12 commented 7 years ago

Closing in favor of #6.