-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add LibSVM #358
Add LibSVM #358
Conversation
Add Windows builds + testing.
|
||
test: | ||
requires: | ||
- python {{ environ['PY_VER'] + '*' }} # [win] |
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.
Thank you @patricksnape. Now you can easily test your windows builds here without waiting for me. Also, thanks for fixing the Unix builds too; that's exactly how I imagined it :) |
There were two test sections which was obviously a mistake.
if errorlevel 1 exit 1 | ||
|
||
REM Install step | ||
copy windows\libsvm.dll %LIBRARY_LIB%\libsvm.dll |
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.
Could you please add if errorlevel 1 exit 1
here too?
Clean was removed before because otherwise it throws an error because some of the directories it tries to delete don't exist. But it does seem to correctly delete the shipped exe and dlls.
But no selector.
Hi! This is the friendly conda-forge-admin automated user. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@patricksnape could you please modify your test section back to this:
and see it will work again? but do not remove Python from build requirements. |
@183amir That works as well. I have tested that before. The issue is that this logic that we can just place it in build and test is not correct because the Python version doesn't seem to propagate properly into the test environment without |
Could you explain further? What do you mean by saying it doesn't propagate? |
Previously it seems the wisdom was that you just turned the features on and then placed |
test: | ||
requires: | ||
- python {{ environ['PY_VER'] + '*' }} # [win] | ||
- python # [win] |
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.
If we can switch back to using environ['PY_VER']
here, but leave everything else alone (including build requirements) and CI passes. I would happily merge this. Thanks for trying this out here so that we can see this doesn't work.
I think what conda is doing is logically correct. You want to build libsvm in a way that is compatible with Python at runtime. Each Python has a different runtime (visual studio runtime). Also, the Python package tracks the runtime feature. |
@183amir Yes but that is not what we have been saying in all the other recipes. We have not been pinning builds Python that do not require Python. If that is what we decide to do then so be it, but I would prefer something like what @jakirkham was suggesting with a proper metapackage used for building. |
Yeah, I don't know about conda doing the right thing here. We have seen this problem with test requirements misbehaving on Windows in lots of places. Though I'm not sure that a bug in conda is really the whole problem. Adding it to run requirements may help, but it just feels wrong. Why is that we must depend on Python for non-Python things? Also, we haven't needed to do this before. We need a better way to track this in Windows. Though if you want to raise this issue on the website issue tracker that seems perfectly fine to me. We do need a better way to fix this. The problem we are seeing here is hardly unique and deserves discussion with the larger community. |
@jakirkham I would not merge this like this. Because I think you will have the same error when you try to actually install the package. |
@183amir It will only fail if you try to install it into an environment where there are no |
Have you run into this issue when trying to install, @patricksnape? If you have, we should probably make it a run requirement (as undesirable as it is). If not, then I think we should just chalk this up as another case where test requirements with conda has a bug. Admittedly this one is a little weirder, but it still feels like the same ilk of problems we have seen before. |
@jakirkham To be honest, I'm pretty pragmatic about this. If everyone is happier with the 100% works but is ugly "add to the run requirements" way - then I will change it to that. We can always revisit when conda build sorts itself out. |
I don't get it. How are you ensuring that 3 different |
Ok, I will leave it up to your discretion, @patricksnape . Personally, I am happy with how it is now.
We are. Take a look at the builds on AppVeyor and you will see this is the case. It is just getting really messed up setting up the testing environment correctly. |
@jakirkham can't we add a meta package in |
That's what I was thinking earlier too. This really seems like a nice way to go as we can use it to properly effect these sorts of changes. |
^ I should add this is a variant based on a suggestion that @pelson made a couple of weeks back. |
Just did a test install locally (in a new environment) and it seems fine to me. So I think this is good as is unless this actually ends up being a problem. Then, if we get that metapackage we can update the feedstock. If it ends up being broken when other packages rely on it somehow then I apologize and at least we learnt something concrete about the resolution system. |
Good enough for me. Satisfied, @183amir? |
I went ahead and created that meta-package (#363). But if this is working and you are ok with it, I'm satisfied. |
Thanks for getting the ball rolling on that @183amir. I added a few comments. Once we get something like that rolling, we will likely want to substituted the Python stuff out of everything that uses it on Windows anyways. |
Thanks everyone for helping get this in tip top shape. |
@183amir Asked me to take over this PR ( #352 ). So I have squashed a bunch of commits and re-added this PR. This should hopefully work on all platforms.