Add Eigen #205

jakirkham · 2016-03-25T21:03:29Z

Adds a recipe to build Eigen. This is based off of the recipe in conda-recipes, but has been cleaned up a bit. Also, did a really rough Windows attempt, which needs help (probably).

This will be needed for OpenCV ( #189 ).

jakirkham · 2016-03-25T21:05:07Z

cc @patricksnape

patricksnape · 2016-03-25T21:11:57Z

recipes/eigen/meta.yaml

+source:
+  fn: eigen-{{ version }}.tar.gz
+  url: http://bitbucket.org/eigen/eigen/get/{{ version }}.tar.gz
+  sha1: 96709ff139c80861e7b2cf65a9dfca8a8cc46eeb


I get 574b0732f7e04ecfa91597afb2f69bc44f3387e6 as the correct SHA1.

Oops, copypasta. Corrected. Thanks. 😄

patricksnape · 2016-03-25T21:12:36Z

Ah great - I was literally just working on this but the tests are failing on my Mac 😖 Thanks!

jakirkham · 2016-03-25T21:14:33Z

Oh, sorry, wasn't trying to steal your thunder. You could PR your changes. Maybe we get a better Windows portion out of it. 😉

jakirkham · 2016-03-25T21:15:53Z

recipes/eigen/bld.bat

+cd build
+
+cmake -DCMAKE_INSTALL_PREFIX=%LIBRARY_PREFIX% -DCMAKE_BUILD_TYPE=Release ..
+cmake --build . --target INSTALL --config Release


Does this do the right thing for install? I copied this from somewhere else. So, I don't know. 😕

Eigen is header-only, right? What is this stuff doing?

Correct it is header only. I copied it from the conda wiki. 😆 Admittedly, it is probably not appropriate. How should it look?

I believe it just sets some defines and then copies the files. It depends on if you want to build and run the tests - they obviously require building.

For reference, my Eigen recipe on Windows looks like:

mkdir build cd build set CMAKE_CONFIG="Release" if "%PY_VER%" == "3.4" ( set GENERATOR=Visual Studio 10 2010 ) else ( if "%PY_VER%" == "3.5" ( set GENERATOR=Visual Studio 14 2015 ) else ( set GENERATOR=Visual Studio 9 2008 ) ) if %ARCH% EQU 64 ( set GENERATOR=%GENERATOR% Win64 ) cmake .. -G"%GENERATOR%" ^ -DCMAKE_BUILD_TYPE=%CMAKE_CONFIG% ^ -DINCLUDE_INSTALL_DIR=%LIBRARY_INC% ^ -DLIB_INSTALL_DIR=%LIBRARY_LIB% ^ -DBIN_INSTALL_DIR=%LIBRARY_BIN% cmake --build . --config %CMAKE_CONFIG% --target ALL_BUILD cmake --build . --config %CMAKE_CONFIG% --target INSTALL

But I honestly can't remember if the LIB_INSTALL_DIR stuff is necessary. Obviously we should use the black magic rather than my generator logic.

Correct, tests must be built, but ctest should do that, no? Admittedly, there may be a bit of configuration needed here. Honestly, I almost question doing tests here at all given no libraries are distributed. Though making sure things were defined correctly is important.

Thanks for the Windows pointers.

So, I have copied your script @patricksnape and pushed that instead. Maybe @msarahan can tell us what black magic is required here.

jakirkham · 2016-03-25T21:52:54Z

recipes/eigen/run_test.bat

@@ -0,0 +1,3 @@
+cl test.cc
+
+test.exe


No clue if this is right. Based on my googling in order to reproduce run_test.sh for Windows, this is what I came up with, but I feel I need to specify the includes somehow.

This appears to be wrong as it causes AppVeyor to fail.

jakirkham · 2016-03-25T22:16:15Z

On my Mac, I am seeing the follow failures in the test suite, gmres_2 and levenberg_marquardt. Circle CI sees only the levenberg_marquardt failure also. Is this the same for you @patricksnape?

jakirkham · 2016-03-25T22:18:39Z

Also, the Mac Travis CI build is definitely timing out. Circle CI ran for ~80mins, but also had a test failure so would take even longer with packaging included. We need to figure out how to make the test suite faster, run less of it, or perhaps drop it entirely.

patricksnape · 2016-03-25T22:25:05Z

@jakirkham Yup - that's what I was seeing too - but I didn't find any bugs related to those failures. Looks like we made need to raise them upstream on Eigen. Perhaps we skip testing for now?

jakirkham · 2016-03-25T22:43:02Z

Initially, I was only seeing bug reports for gmres_2, but now I have found the ticket that is closest to ours. Though they have an additional test failure that I am not seeing. Maybe you are seeing this?

jakirkham · 2016-03-25T22:48:57Z

recipes/eigen/bld.bat

+ -DBIN_INSTALL_DIR=%LIBRARY_BIN%
+
+cmake --build . --config %CMAKE_CONFIG% --target ALL_BUILD
+ctest


Apparently, this won't make the tests run. Maybe it needs to after the last cmake call.

jakirkham · 2016-03-25T22:49:33Z

Perhaps we skip testing for now?

Agreed. I don't really see anyway around this. The test suite is just way to long. That or we try to increase the number of cores for testing up to some limit. Though I don't know if it is worth the effort if we are only going to see known test failures. Plus, it may just make the CIs crash for different reasons.

jakirkham · 2016-03-25T22:57:37Z

recipes/eigen/bld.bat

+
+if %ARCH% EQU 64 (
+    set GENERATOR=%GENERATOR% Win64
+)


Do we still need all this generator stuff if we aren't doing testing? Does this affect how things are configured by Eigen?

jakirkham · 2016-03-26T02:23:08Z

recipes/eigen/run_test.bat

@@ -0,0 +1,3 @@
+C:\MinGW\bin\g++ -I%LIBRARY_INC% -o test test.cc


Could someone explain to me how to get this compiled? Either through g++ or some standard Windows mechanism. As it's just a test, it shouldn't matter which one is picked as long as it does compile.

Question: Here we are using AppVeyor's MinGW, but there is also a mingw package in the default channel. Which one should we use/recommend? Or it does not matter?

Build the 7 basic tests. Remove the MinGW testing for now because I'm not sure building with MingGW really proves anything if some tests pass and then files are where we expect them.

Wrong filename and testing syntax

Small number of tests - seems sensible.

patricksnape · 2016-03-31T11:38:40Z

OK I put a PR up against your fork that shows how the fix might look - then we can merge it (if you like it) and check it actually works!

This has been a good exercise in creating a solid build and we should sort this issue, however, ironically, it turns out that we only actually need one build...

Fix Appveyor VS9 Express x64 builds

conda-forge-admin · 2016-03-31T14:42:12Z

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

jakirkham · 2016-03-31T14:45:12Z

Merged @patricksnape's changes from this PR ( jakirkham#6 ). Thanks @patricksnape.

Let's discuss them, see how this works, and figure out how we want to integrate this. 😄

jakirkham · 2016-03-31T14:50:45Z

OK I put a PR up against your fork that shows how the fix might look - then we can merge it (if you like it) and check it actually works!

Absolutely! Let's see indeed. 😄

This has been a good exercise in creating a solid build and we should sort this issue, however, ironically, it turns out that we only actually need one build...

Not too surprised by that response, honestly. Though it seemed like this was the best place to try out the modifications that you had in mind as it literal has no effect other than the tests.

jakirkham · 2016-03-31T17:07:16Z

The previous build in the matrix that was failing is now passing. Thoughts about incorporation, @pelson?

jakirkham · 2016-03-31T18:23:02Z

Passing on all CIs with these build infrastructure fixes. Thoughts about their incorporation into staged-recipes and conda-smithy?

conda-forge-admin · 2016-03-31T19:44:12Z

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

conda-forge-admin · 2016-03-31T20:22:23Z

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

jakirkham · 2016-03-31T20:27:42Z

Thanks again for your changes, @patricksnape. We have split them into the CI infrastructure with this PR ( #251 ). I have also merged that PR into this one (Eigen's) so that we can clean that stuff out of the recipe. Right now, we are testing it to make sure the structure works correctly and then will merge it into staged-recipes and propose similar logic for conda-smithy. Will keep you apprised. Feedback welcome.

jakirkham · 2016-03-31T22:36:06Z

Still passing the relevant case. Though want to wait until AppVeyor finishes.

jakirkham · 2016-03-31T23:11:15Z

Integration of the VS 2008 64-bit workaround by @patricksnape is proposed in this PR ( conda-forge/conda-smithy#107 ).

jakirkham · 2016-03-31T23:52:53Z

Saving this to test conda-smithy changes if/when they are released.

patricksnape · 2016-04-01T05:44:02Z

Right - now - although this was a good test bed for Appveyor - the guys on the Eigen mailing list replied and confirmed that we actually don't need all the Windows builds 😆. We just need one build as all CMake does is copy the files! I will put a PR up for that this evening.

jakirkham · 2016-04-01T05:45:48Z

Well, I plan on merging to test the conda-smithy stuff so maybe hold off on a PR until it is a feedstock. Want to make sure we really have solved this in our infrastructure.

jakirkham · 2016-04-01T13:33:17Z

Alright, think I will do the re-rendering manually.

jakirkham · 2016-04-01T13:34:37Z

Thanks everyone for your helpful reviews. Also, thanks @patricksnape and @msarahan for your help in getting Windows working here and tracking down relevant issues.

jakirkham · 2016-04-01T14:15:57Z

🎉 It's in! 🎉

Thanks @patricksnape for putting the Windows stuff together.

Mac and Linux builds are released. Windows is pending. Also, @patricksnape, you will be asked if you would like to join conda-forge. In particular, you will be added to the eigen team, which gives commit privileges on this feedstock/repo. This is useful for making tweaks, new releases, merging suggested changes, and the like to eigen feedstock, which will contain these recipes. All of the CI stuff is automatically maintained (may need to merge a PR to update though). If you have any questions, please let me know.

Also, I have re-rendered the feedstock with a development version conda-smithy to get your nice fixes for VC2008 support of 64-bit compilation. If I haven't added a typo, I expect this will go smoothly and generate Windows binaries.

patricksnape · 2016-04-01T14:25:30Z

Great work @jakirkham! Thanks for the conda-forge invite 😄

jakirkham · 2016-04-01T14:43:05Z

Great work @jakirkham!

Thank you for all the hard work on Windows. This will be very helpful. 👍

Thanks for the conda-forge invite 😄

You'll have to thank our bot. 😆

jakirkham mentioned this pull request Mar 25, 2016

OpenCV 2.x #189

Merged

patricksnape reviewed Mar 25, 2016
View reviewed changes

jakirkham force-pushed the add_eigen branch from cc1b8fa to 294994b Compare March 25, 2016 21:13

jakirkham reviewed Mar 25, 2016
View reviewed changes

jakirkham added 2 commits March 25, 2016 17:49

eigen: Add recipe to build Eigen.

cdbeef3

eigen: Use @patricksnape's Windows script.

19d19ac

jakirkham force-pushed the add_eigen branch from 294994b to 19d19ac Compare March 25, 2016 21:50

jakirkham reviewed Mar 25, 2016
View reviewed changes

eigen: Drop testing steps as they are way too long for CI.

a097e24

jakirkham reviewed Mar 25, 2016
View reviewed changes

jakirkham force-pushed the add_eigen branch 2 times, most recently from d209561 to b05845a Compare March 26, 2016 00:51

eigen: Test with MinGW instead.

b05845a

jakirkham reviewed Mar 26, 2016
View reviewed changes

pelson changed the title ~~Add Eigen~~ WIP: Add Eigen Mar 26, 2016

patricksnape added 3 commits March 27, 2016 17:24

Update to black magic Windows

f9bcb3a

Build the 7 basic tests. Remove the MinGW testing for now because I'm not sure building with MingGW really proves anything if some tests pass and then files are where we expect them.

Fix obvious unix mistakes

1b1ce30

Wrong filename and testing syntax

Add tests for basicstuff

421e1bc

Small number of tests - seems sensible.

Might as well make the setup_x64 script do everything

98d3db5

Merge pull request #6 from patricksnape/add_eigen

fc99122

Fix Appveyor VS9 Express x64 builds

jakirkham mentioned this pull request Mar 31, 2016

Fix Appveyor VS9 Express x64 builds jakirkham/staged-recipes#6

Merged

jakirkham mentioned this pull request Mar 31, 2016

Incorporate VS 2008 64-bit workaround #251

Merged

patricksnape and others added 2 commits March 31, 2016 16:05

CI: Incorporate workaround so as to support 64-bit VS 2008.

2e5a2bd

Merge branch 'ci_vs2008_x64' into add_eigen

1b2e906

jakirkham force-pushed the add_eigen branch from 8b79aac to 1b2e906 Compare March 31, 2016 20:22

jakirkham changed the title ~~WIP: Add Eigen~~ Add Eigen Mar 31, 2016

jakirkham mentioned this pull request Mar 31, 2016

WIP: CI: Incorporate workaround so as to support 64-bit VS 2008 conda-forge/conda-smithy#107

Closed

jakirkham merged commit a3ad28d into conda-forge:master Apr 1, 2016

jakirkham deleted the add_eigen branch April 1, 2016 13:33

		@@ -0,0 +1,3 @@
		C:\MinGW\bin\g++ -I%LIBRARY_INC% -o test test.cc

Add Eigen #205

Add Eigen #205

Conversation

jakirkham commented Mar 25, 2016

jakirkham commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricksnape commented Mar 25, 2016

jakirkham commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Mar 25, 2016

jakirkham commented Mar 25, 2016

patricksnape commented Mar 25, 2016

jakirkham commented Mar 25, 2016

Choose a reason for hiding this comment

jakirkham commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricksnape commented Mar 31, 2016

conda-forge-admin commented Mar 31, 2016

jakirkham commented Mar 31, 2016

jakirkham commented Mar 31, 2016

jakirkham commented Mar 31, 2016

jakirkham commented Mar 31, 2016

conda-forge-admin commented Mar 31, 2016

conda-forge-admin commented Mar 31, 2016

jakirkham commented Mar 31, 2016

jakirkham commented Mar 31, 2016

jakirkham commented Mar 31, 2016

jakirkham commented Mar 31, 2016

patricksnape commented Apr 1, 2016

jakirkham commented Apr 1, 2016

jakirkham commented Apr 1, 2016

jakirkham commented Apr 1, 2016

jakirkham commented Apr 1, 2016

patricksnape commented Apr 1, 2016

jakirkham commented Apr 1, 2016