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

Windows support and Ipopt 3.12.13 #34

Closed
wants to merge 23 commits into from

Conversation

richardotis
Copy link
Contributor

Checklist

  • Used a fork of the feedstock to propose changes
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Closes #1

This PR is an attempt to get Windows support working for ipopt using the m2w64 toolchain and cmake. The cmake scripts are based on the awesome efforts of @ycollet to ship CMake scripts for the whole Coin-OR ecosystem: https://github.com/ycollet/coinor-cmake
The scripts here are lightly modified to address some path differences, and for stability across future versions of Ipopt.

  1. Do we need to switch all three platforms to CMake here, or is it okay to only use CMake for Windows?
  2. I'm using m2w64-toolchain here, even though it's deprecated, because the compiler('m2w64_c') syntax didn't work locally when I ran conda build recipe -m .ci_support\win_c_compilervs2015cxx_compilervs2015.yaml. Help would be appreciated here.
  3. The CMake scripts from @ycollet are LGPL licensed (for which I don't really understand the implications in the context of build scripts). I think that accepting this PR would mean this feedstock (but not the actual package) would become LGPL licensed. Someone from the @conda-forge/core team should probably chime in here.
  4. There are likely a bunch of unnecessary CMake support macros here that can be deleted.
  5. The requirements in the recipe are not fully optimized, and I am not 100% clear on the differences between build, host and run requirements (I read the documentation and still don't quite get it).
  6. Ipopt has a bunch of build-time options, including for AMPL, HSL, Metis, performance options like fastmath and MKL, as well as minor(?) ones like link-time optimization. Which of these should be exposed as features in the recipe?
  7. There is a lot of path- and link-hacking in the bld.bat to get the compiler to find and link all the needed libraries. I am not sure how portable it is across Windows version, nor am I clear on why CMake is not doing this automatically (it's there in the CMake scripts).

@conda-forge-linter
Copy link

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.

@chrisdembia
Copy link

Neat! Where can I learn more about the m2w64 toolchain? Which compilers are used? Will this build of Ipopt be usable with Visual Studio?

@richardotis
Copy link
Contributor Author

@chrisdembia I don't know the best place; everything I've learned about m2w64 has been random conda-forge feedstock issue trackers. There is some discussion here: ContinuumIO/anaconda-issues#1484 The toolchain package itself is deprecated but the technology has been rolled into conda-forge's official compiler support, so I think it's still okay to use (but it has a new name).

MSVC does not appear to compile Ipopt (I tried that first), and it is not officially supported upstream. The compiler stack used here is essentially MinGW.

@chrisdembia
Copy link

Okay thank you. I'm hoping to use Ipopt in a C++ project compiled with Visual Studio, and my understanding is I cannot use libraries compiled with MinGW.

I think Ipopt can be compiled with Visual Studio; it's just that its MUMPS dependency is written in Fortran and needs to be compiled with gfortran or another open-source Fortran compiler.

Anyway, I was just curious. You can ignore my comments.

@richardotis
Copy link
Contributor Author

@conda-forge-admin, please rerender

@richardotis
Copy link
Contributor Author

@conda-forge/core It looks to me like something weird is happening in the Linux build infrastructure. Configure seems to be pulling in the link flag -lm' (note the extra quote character). Is that normal? If not, it would explain the strange linking to Fortran libraries from C fails message we're getting here.

@richardotis
Copy link
Contributor Author

I can reproduce the Linux error locally with conda build recipe -m .ci_support/linux_.yaml

@richardotis
Copy link
Contributor Author

richardotis commented Dec 23, 2019

Here's an excerpt from the config.log:

configure:2027: $? = 0
configure:2029: /home/rotis/anaconda3/conda-bld/ipopt_1577132049035/_build_env/bin/x86_64-conda_cos6-linux-gnu-cc -v </dev/null >&5
Reading specs from /home/rotis/anaconda3/conda-bld/ipopt_1577132049035/_build_env/bin/../lib/gcc/x86_64-conda_cos6-linux-gnu/7.3.0/specs
COLLECT_GCC=/home/rotis/anaconda3/conda-bld/ipopt_1577132049035/_build_env/bin/x86_64-conda_cos6-linux-gnu-cc
COLLECT_LTO_WRAPPER=/home/rotis/anaconda3/conda-bld/ipopt_1577132049035/_build_env/bin/../libexec/gcc/x86_64-conda_cos6-linux-gnu/7.3.0/lto-wrapper
Target: x86_64-conda_cos6-linux-gnu
Configured with: /home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/.build/x86_64-conda_cos6-linux-gnu/src/gcc/configure --build=x86_64-build_pc-linux-gnu --host=x86_64-build_pc-linux-gnu --target=x86_64-conda_cos6-linux-gnu --prefix=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/gcc_built --with-sysroot=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/gcc_built/x86_64-conda_cos6-linux-gnu/sysroot --enable-languages=c,c++,fortran,objc,obj-c++ --with-pkgversion='crosstool-NG 1.23.0.452-d158' --enable-__cxa_atexit --disable-libmudflap --enable-libgomp --disable-libssp --enable-libquadmath --enable-libquadmath-support --enable-libsanitizer --enable-libmpx --with-gmp=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/.build/x86_64-conda_cos6-linux-gnu/buildtools --with-mpfr=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/.build/x86_64-conda_cos6-linux-gnu/buildtools --with-mpc=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/.build/x86_64-conda_cos6-linux-gnu/buildtools --with-isl=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/.build/x86_64-conda_cos6-linux-gnu/buildtools --enable-lto --with-host-libstdcxx='-static-libgcc -Wl,-Bstatic,-lstdc++,-Bdynamic -lm' --enable-threads=posix --enable-target-optspace --enable-plugin --enable-gold --disable-nls --disable-multilib --with-local-prefix=/home/conda/feedstock_root/build_artifacts/ctng-compilers_1570984655235/work/gcc_built/x86_64-conda_cos6-linux-gnu/sysroot --enable-long-long --enable-default-pie
Thread model: posix
gcc version 7.3.0 (crosstool-NG 1.23.0.452-d158) 

Notice we're getting back --with-host-libstdcxx='-static-libgcc -Wl,-Bstatic,-lstdc++,-Bdynamic -lm'

This looks like a bad interaction between the gcc output and Ipopt's configure script. This is a known upstream bug for a long time: https://projects.coin-or.org/Ipopt/ticket/215 - (So how did the Linux build ever work...?)

@richardotis
Copy link
Contributor Author

This workaround commit was just a test to see if the Linux build will work if you hardcode FLIBS. The real solution (which should probably become its own PR) will be to patch the upstream BuildTools/coin.m4 to pass a correctly formed FLIBS. We're on the hook to do this because the bug is marked wontfix upstream, and the next version of Ipopt actually has a build system overhaul, so any upstream patch would probably be moot.

I'd like to get functioning Ipopt 3.12.x for all platforms before dealing with 3.13, though.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

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.

@richardotis
Copy link
Contributor Author

@conda-forge-admin, please rerender

@moorepants
Copy link
Contributor

The CMake scripts from @ycollet are LGPL licensed (for which I don't really understand the implications in the context of build scripts). I think that accepting this PR would mean this feedstock (but not the actual package) would become LGPL licensed. Someone from the @conda-forge/core team should probably chime in here.

I'm not sure if we are supposed to license the build scripts anything other than the default. But I am sure that other feedstocks likely contain patches that are taken from non-BSD-3 licensed code.

@@ -8,6 +8,7 @@ mkdir build
cd build

../configure \
FLIBS="-L$PREFIX/lib -L$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda_cos6-linux-gnu/7.3.0 -L$BUILD_PREFIX/bin/../lib/gcc -L$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda_cos6-linux-gnu/7.3.0/../../../../x86_64-conda_cos6-linux-gnu/lib/../lib -L$BUILD_PREFIX/bin/../x86_64-conda_cos6-linux-gnu/sysroot/lib/../lib -L$BUILD_PREFIX/bin/../x86_64-conda_cos6-linux-gnu/sysroot/usr/lib/../lib -L$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda_cos6-linux-gnu/7.3.0/../../../../x86_64-conda_cos6-linux-gnu/lib -L$BUILD_PREFIX/bin/../x86_64-conda_cos6-linux-gnu/sysroot/lib -L$BUILD_PREFIX/bin/../x86_64-conda_cos6-linux-gnu/sysroot/usr/lib -lrt -lgfortran -lm -lquadmath" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? Seems like these should be discovered rather than specified explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That shouldn't be here and looks like a rebase error on my part.

@moorepants
Copy link
Contributor

@isuruf Would you be willing to answer our questions here and double check this windows build?

@isuruf
Copy link
Member

isuruf commented Jan 27, 2020

Is this mingw approach used in other conda-forge packages and do the primary conda-forge folks approve of this method?

Only rarely. Most packages use MSVC.

Will downstream packages have to deal with any of the mingw toolchain and/or are there build/install consequences for those packages?

The C runtime that MinGW and MSVC are using are slightly different. Therefore, there might be incompatibilities. (segfaults when you try to free an object created in a different C runtime)
So, downstream packages will have to be built with MinGW too.

@moorepants
Copy link
Contributor

Thanks! There is also the list of questions in the very first comment that we don't have answers to.

@isuruf
Copy link
Member

isuruf commented Jan 27, 2020

Also, the C++ runtimes are completely different.

@isuruf
Copy link
Member

isuruf commented Jan 27, 2020

  1. No. Using CMake just for Windows is fine and preferred to keep backwards compatibility.
  2. You need to rerender after using the compiler('m2w64_c') syntax
  3. That's fine. You can keep the license of the scripts in the recipe.
  4. Those are probably bugs in the cmake scripts.

@isuruf
Copy link
Member

isuruf commented Jan 27, 2020

Since there's CMake support, why not try VS2015?

@richardotis
Copy link
Contributor Author

@isuruf One part of the ipopt build process requires a working Fortran compiler, at least for the 3.12.x versions. Also, when I naively tried changing the build script to create CMake VS Makefiles, I received errors unrelated to a missing Fortran compiler. Generally speaking, upstream seemed to recommend the MSYS/MinGW option for building: https://projects.coin-or.org/Ipopt/browser/stable/3.12/Ipopt/doc/documentation.pdf?format=raw The story might have changed for 3.13, since they rewrote the build system, but I haven't looked at it yet.

I'm not saying it's impossible, but it's unclear how much work it would be.

@moorepants
Copy link
Contributor

We are are using flang in th slycot feedstock with cmake:

https://github.com/conda-forge/slycot-feedstock

Maybe that's possible?

@richardotis
Copy link
Contributor Author

These new commits for a MSVC-compatible build depend on conda-forge/mumps-feedstock#56

Even with that PR, locally I get some test failures for the built-in examples. However, the majority of them pass and I am not sure if some numerical instability is to be expected here.

@nniclausse
Copy link

Hi, i tested the 3.12.13 package from the pycalphad channel, it does not include the file IpoptConfig.h which is needed by IpDebug.hpp, IpJournalist.hpp and IpTypes.hpp

@richardotis
Copy link
Contributor Author

@nniclausse Are those files needed by downstream packages, or only at build time?

@nniclausse
Copy link

@nniclausse Are those files needed by downstream packages, or only at build time?

I'm trying to build an app that use ipopt, this app includes IpTNLP.hpp, which requires IpUtils.hpp -> IpTypes.hpp -> IpoptConfig.h -> config_ipopt_default.h

@richardotis
Copy link
Contributor Author

Ah ok, I think the reason I didn't catch it is because I was testing against the C interface without making sure C++ still worked.

@traversaro
Copy link
Contributor

Hi @richardotis, I am interesting in helping to get this merged, any suggestion on what are the next steps that need to be addressed? Thanks!

@traversaro traversaro mentioned this pull request Oct 30, 2020
5 tasks
@traversaro
Copy link
Contributor

I guess we can close this PR as it has been superseded by #47 @conda-forge/ipopt .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please support Windows
9 participants