-
Notifications
You must be signed in to change notification settings - Fork 56
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
Resolve compatibility issues with windows #49
Conversation
There are some Travis errors with these changes:
|
It appears to be working on Windows 10 64 bits - v
Followed the instructions and it appears to be finally working under Windows 10 (tested on 64bits version 1909, build 18363.476). |
@moorepants Yep I saw that. It does exactly the opposite of what it did on windows before. I don't really know linux so I am not sure, but I would expect both of them to behave similarly on imports. I will try to find something that works on both systems. |
|
I tried something, the build structure seemed somehow different between linux and windows setup, so I tried to harmonize that. Because I don't have access to a linux system I couldn't test the commit beforehand so the travis builds still seem to fail at some point but the tests seem to execute correctly though. I can also edit the windows structure to be similar to linux and it should pass, but somehow I feel like the windows structure makes more sense. I saw that it was planned to rename the main module cyipopt, so I think a more proper and general fix could be done at this moment. |
I changed the setup structure for windows instead of linux this time and reverted the other changes. This way, the checks pass and it also works on windows (for me at least) with only minor modifications compared to the current implementation. |
@apommel I definitely agree with cyipopt being the name of the package. I'm having issues when packaging a module with this package as dependency (because the conda-forge channel already has a package named ipopt). I would appreciate if this naming issue could be solved or if you have any idea/tips on how to solve this. |
The naming is the result of how Cython works. The discussion for changing the distribution and/or package names should be handled in another issue. Note that it has been discussed in #14. |
IPOPT_INCLUDE_DIRS = ['include/coin', np.get_include()] | ||
IPOPT_LIBS = ['Ipopt-vc8', 'IpOptFSS', 'IpOpt-vc10'] | ||
IPOPT_LIB_DIRS = ['lib/x64/ReleaseMKL'] | ||
IPOPT_DLL = ['IpOptFSS.dll', 'Ipopt-vc8.dll', 'IpOpt-vc10.dll', 'libiomp5md.dll', 'msvcp100.dll', 'msvcr100.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.
Do you know what version of IPOPT made these changes? It could be helpful to add that we only support ipopt >= some version for windows in the README.
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.
From the official builds available here, the directory is organized this way starting Ipopt 3.10.1.
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.
Can you note this in the README? i.e. that for Windows we only support Ipopt >= 3.10.1.
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.
Added in latest commit.
I have seen this issue, that is why I mentioned that this should be better looked at at this time. For this PR, assuring compatibility with windows while keeping the current implementation is enough I think. |
@apommel Thanks for the changes. I'm going to merge this, but I'd like to get a windows CI setup to test this, before a new release happens. |
Attempt at resolving both setup compatibility problems with windows #45 #46 for the latest Ipopt dll build available (3.11.0) and windows import issues #21