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

Added a build recipe for Simbody 3.5.3. #1076

Merged
merged 30 commits into from
Jul 22, 2016

Conversation

moorepants
Copy link
Contributor

@moorepants moorepants commented Jul 17, 2016

This is a minimal Simbody installation for all three platforms.

TODO

  • Figure out the activate error on Windows.
  • Deal with the Mac rpath warning (either apply a patch from Simbody dev or ...)

This is a minimal Simbody installation for all three platforms.
@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/libgfortran, recipes/pytrie, recipes/simbody, recipes/subprocess32) and found some lint.

Here's what I've got...

For recipes/simbody:

  • The recipe must have some tests.

@moorepants
Copy link
Contributor Author

@chrisdembia Have a look at this. I'm going to try to get everything compiled for conda with Simbody and Opensim.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/simbody) and found some lint.

Here's what I've got...

For recipes/simbody:

  • The recipe must have some tests.

- blas # [not win]
- lapack # [not win]
- vc 14 # [win]

Copy link
Member

Choose a reason for hiding this comment

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

@moorepants you can add a simple "file exists" test here. See an example in:

https://github.com/conda-forge/hdfeos2-feedstock/blob/master/recipe/meta.yaml#L26-L27

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

@moorepants
Copy link
Contributor Author

@sherm1 conda-forge does not yet have libxi and libxmu built as binaries, so this doesn't include those optional dependencies. I'd like to make conda packages here for libxi and libxmu so that the visualizer works. Do these two libraries build easily on Windows? How do you typically distrubute the visualizer dependencies for Simbody on Windows?

Also, how can I ensure that Simbody links against a lapack and blas that we have installed. Is that possible or do you ship those with Simbody and we have to use your version?

@chrisdembia
Copy link
Contributor

chrisdembia commented Jul 18, 2016

Thank you for doing this!

@klshrinidhi may be interested.

How do you typically distrubute the visualizer dependencies for Simbody on Windows?

It seems that there are no visualizer dependencies on Windows.

Also, how can I ensure that Simbody links against a lapack and blas that we have installed. Is that possible or do you ship those with Simbody and we have to use your version?

Simbody does come with blas/lapack on Windows but it is fine to use other blas/lapack. This is controlled by setting the cmake variable BUILD_USING_OTHER_LAPACK. I think it should be set to a semicolon-separated list of the libraries to use (either the names or the full paths?).

@moorepants do you plan on packaging released versions of Simbody/OpenSim? I'm not sure it is a good idea to package the development version of OpenSim; it is changing a lot and should not be used for research in its current state. IMO it's fine to package the current development version of Simbody (i.e., master branch). EDIT: I can see now that you only plan on packaging released versions.


# NOTE: This installs the minimum dependencies for Simbody, i.e., no
# visualizer. I'm also not completely sure if blas and lapack are necessary for
# the unix builds because they are supposedly included with Simbody. Once
Copy link
Contributor

Choose a reason for hiding this comment

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

blas/lapack are not included on UNIX; they are only included on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll remove this note.

@moorepants
Copy link
Contributor Author

@isuruf The build is failing on non py35 instances on appveyor. What do you do to handle that?


build:
number: 0
skip: true # [win and py35]
Copy link
Member

Choose a reason for hiding this comment

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

This should be win and not py35

@moorepants
Copy link
Contributor Author

moorepants commented Jul 18, 2016

@chrisdembia

Thank you for doing this!

NP, the combination of a matured conda-forge build system and inspiration at SciPy 2016 finally got me to do it.

It seems that there are no visualizer dependencies on Windows.

Ok. That makes it easy.

Simbody does come with blas/lapack on Windows but it is fine to use other blas/lapack. This is controlled by setting the cmake variable BUILD_USING_OTHER_LAPACK. I think it should be set to a semicolon-separated list of the libraries to use (either the names or the full paths?).

I'll use Simbody's lapack for Windows and the conda provided ones for Mac and Linux.

@moorepants do you plan on packaging released versions of Simbody/OpenSim? I'm not sure it is a good idea to package the development version of OpenSim; it is changing a lot and should not be used for research in its current state. IMO it's fine to package the current development version of Simbody (i.e., master branch). EDIT: I can see now that you only plan on packaging released versions.

Correct, for now I'll only package released versions, but conda-forge supports having dev builds on pre-release channels. We could set that up too later.

@moorepants moorepants closed this Jul 18, 2016
@moorepants moorepants reopened this Jul 18, 2016
@@ -0,0 +1,8 @@
mkdir build
cd build
cmake -G "%CMAKE_GENERATOR%" -DBUILD_VISUALIZER=OFF -DCMAKE_INSTALL_PREFIX="%LIBRARY_PREFIX%"
Copy link
Member

Choose a reason for hiding this comment

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

I think %CMAKE_GENERATOR% can't be quoted.

@jakirkham
Copy link
Member

jakirkham commented Jul 19, 2016

I was about to recommend that we set detect_binary_files_with_prefix to true in the build section.

Interesting. I'm not sure that feature does exactly what we want, since it's the install directory that gets baked in, not the build directory. Though these terms might mean different things to Conda vs CMake.

Ah, but that's not the case. Everything gets installed into a build prefix. Only after that has occurred is this detected and fixed. Then it is packaged into a tarball for distribution. So I do think this will give us what we want.

@moorepants
Copy link
Contributor Author

I'm trying the flag locally, will push if it works.

@chrisdembia
Copy link
Contributor

Everything gets installed into a build prefix

Ah excellent; thank you for explaining.

@moorepants
Copy link
Contributor Author

Ok, the flag in the meta.yaml files works. I can now run the examples for simbody within a conda env.

One more thing though. I had to manually set LD_LIBRARY_PATH to /home/moorepants/miniconda/envs/simbody-test/lib for things to properly link. @jakirkham Is there something in conda that sets this for you or rewrites paths so that it isn't needed? It'd be nice if users didn't have to do this.

Also, when in a conda env, will the compiler look into /home/moorepants/miniconda/envs/simbody-test/include for headers automatically? i.e. does conda help redirect things there?

@jakirkham
Copy link
Member

So, it sounds like you were trying to build something else after building Simbody, correct? Possibly something that was linking to it? Could you please explain how you were doing this build?

@moorepants
Copy link
Contributor Author

Yes, simbody is a library and you typically write a C++ file with your simulation model and then link to the simbody lib when compiling. Here is what I did:

$ conda create -n simbody-test simbody
$ source activate simbody-test
(simbody-test)$ cp simbody-example-from-readme example.cpp
(simbody-test)$ # create simple CMakeLists.txt like https://github.com/simbody/simbody/blob/master/cmake/SampleCMakeLists.txt
(simbody-test)$ export LD_LIBRARY_PATH=~/miniconda/envs/simbody-test/lib
(simbody-test)$ cmake .
(simbody-test)$ make
(simbody-test)$ ./example

This will open up a simple GUI that shows the example animation of the system. I'm curious if there is a way to avoid have to set LD_LIBRARY_PATH. If I installed simbody system wide it would install to /usr/local by default and I don't think I'd have to set LD_LIBRARY_PATH because the lib is in a standard location. Does conda magically make it's environment the "standard location"?

@jakirkham
Copy link
Member

I think you need to specifying some more info to CMake. For instance, where the libraries live and similar.

@moorepants
Copy link
Contributor Author

Should I disable the windows builds and we merge this? If so, what's the best way to do that?


build:
number: 200
skip: true # [win and not py35]
Copy link
Member

Choose a reason for hiding this comment

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

Change the selector win and not py35 -> win.

Copy link
Member

Choose a reason for hiding this comment

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

Probably a [skip appveyor] would be nice too.

Copy link
Member

Choose a reason for hiding this comment

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

...the last part refers to the commit message if that was not obvious. Sorry it was a bit vague.

@jakirkham
Copy link
Member

SGTM. The changes to skip Windows are noted above.

Great work on this all. Will be a very nice addition to conda-forge.

- python 3.5.* # [win and py35]
commands:
- if not exist %LIBRARY_INC%\\simbody\\Simbody.h exit 1 # [win]
- test -f $PREFIX/include/simbody/Simbody.h # [unix]
Copy link
Member

Choose a reason for hiding this comment

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

We should probably have more file exists tests. Some tests for the libraries and binaries would be good.

@jakirkham
Copy link
Member

It seems to be using lib64 on Linux. We need to fix it to use lib or move the contents over.

@chrisdembia
Copy link
Contributor

I think you can use CMAKE_INSTALL_LIBDIR to control that.

@chrisdembia
Copy link
Contributor

We use CMake's GNUInstallDirs to determine the correct platform-dependent install directories.

@jakirkham
Copy link
Member

We use CMake's GNUInstallDirs to determine the correct platform-dependent install directories.

So will overriding with CMAKE_INSTALL_LIBDIR still be respected then?

@chrisdembia
Copy link
Contributor

Depends on if those variables are cmake cache variables or not. I think changes made by the user will be respected but I am not certain.

@moorepants
Copy link
Contributor Author

That cmake flag didn't seem to force the files to be in lib instead of lib64.

@moorepants
Copy link
Contributor Author

Oh sorry, i must have been looking at an old ci run. It looks like it worked.

@jakirkham
Copy link
Member

LGTM

- if not exist %LIBRARY_INC%\\simbody\\SimTKlapack.h exit 1 # [win]
- if not exist %LIBRARY_INC%\\simbody\\SimTKmath.h exit 1 # [win]
- if not exist %LIBRARY_INC%\\simbody\\SimTKsimbody_aux.h exit 1 # [win]
- if not exist %LIBRARY_INC%\\simbody\\SimTKsimbody.h exit 1 # [win]
Copy link
Member

Choose a reason for hiding this comment

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

Windows failure was because of this difference, https://github.com/simbody/simbody/blob/master/CMakeLists.txt#L567-L579

Headers and visualizer are installed in different directories than Unixes, but IMO, they shouldn't.

Copy link
Contributor

@chrisdembia chrisdembia Jul 22, 2016

Choose a reason for hiding this comment

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

but IMO, they shouldn't.

You mean for conda, specifically? I think it makes sense for conda to use the same installation layout for all platforms, but in general, different platforms have different requirements and conventions.

I'm fine with having Simbody's SIMBODY_VISUALIZER_REL_INSTALL_DIR and SIMBODY_INCLUDE_INSTALL_DIR being cache variables so that packagers can customize their values.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Some of the reasons mentioned there doesn't apply to conda, but making those two cache variables should fix it.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, it should not be different in general. For eg: a user would do,

#include <simbody/Simbody.h>

in user program. This would work in Unixes, but will not in Windows because Simbody.h is installed in C:\Program Files\simbody\include\Simbody.h

Copy link
Contributor

@chrisdembia chrisdembia Jul 22, 2016

Choose a reason for hiding this comment

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

A user should not use #include <simbody/Simbody.h>, and in general this won't work unless they install Simbody into /usr. As shown in Simbody examples, the user should use #include "Simbody.h" (or #include <Simbody.h>). This will work cross-platform because of the way we create a cmake Config file / pkg-config file. Both of these files are configured by CMake to provide the appropriate include dir for the platform you're on.

We expect that people building a project that uses Simbody will use CMake to set up the appropriate compiler/linker flags, etc. If a downstream project to Simbody chooses not to use CMake (or pkg-config) to set up the flags, then yes, the project will have to specify their own platform-dependent behavior.

On UNIX, it is conventional to install all headers inside a <project-name> folder (e.g., $prefix/include/simbody), since the project is often installed in a system-wide directory. This is not the case on Windows (aside from conda), so an extra simbody directory would be weird.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Project config file does include $PREFIX/include/simbody in it, so it should be good. pkg-config is not though, but it's unrelated to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll merge this as is fr Windows and once the conda activate bug is addressed we can get the Windows build working.

@jakirkham jakirkham merged commit 1777bd8 into conda-forge:master Jul 22, 2016
@jakirkham
Copy link
Member

Thanks @moorepants for putting this together. Also thanks @chrisdembia, @isuruf, and @kmuehlbauer for your helpful reviews. Now let's get this into a feedstock. 😄

@chrisdembia
Copy link
Contributor

🎆 Thanks everyone.

@moorepants moorepants deleted the simbody branch April 5, 2018 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants