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

Fix for #38: use CMAKE_INSTALL_PREFIX #40

Merged
merged 2 commits into from Oct 3, 2013

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Oct 2, 2013

Except for the root CMakeLists.txt, it's a search and replace of SimTK_INSTALL_PREFIX to CMAKE_INSTALL_PREFIX. For the root CMakeLists.txt, I remove the block of code that overwrites CMAKE_INSTALL_PREFIX with the value of SimTK_INSTALL_PREFIX, and add a different block that will copy SimTK_INSTALL_PREFIX into CMAKE_INSTALL_PREFIX under certain conditions for backward compatibility.

I have tested this on OSX, but I still need to test on my Ubuntu machine. If someone can test this on Windows I would appreciate it.

SimTK_INSTALL_PREFIX was previously used for specifying the
install prefix and CMAKE_INSTALL_PREFIX. This commit changes
to use CMAKE_INSTALL_PREFIX instead.
@sherm1
Copy link
Member

sherm1 commented Oct 2, 2013

Thanks, Steve! Travis grabbed this and built it successfully on Ubuntu with both gcc and clang. I will try it on Windows.

OK, I tried this on 64 bit Windows 7 building 32 bit and 64 bit Simbody with Visual Studio 10. CMake 2.8.10.2 managed to correctly initialize CMAKE_INSTALL_PREFIX with c:/program files(x86) when I built 32 bit and c:/program files when I built 64 bit, which are the correct defaults.

@chrisdembia
Copy link
Member

The Travis test is not sufficient, because it doesn't do an install. It builds and runs the tests right in the source tree.

I'll try on my Ubuntu machine.

"Install path prefix." FORCE)
mark_as_advanced(CMAKE_INSTALL_PREFIX)
ENDIF()
ENDIF()
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that this section is actually working. The intent was to allow cmake .. -DSimTK_INSTALL_PREFIX=${PREFIX}, but I think it's not working.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that is because CMAKE_INSTALL_PREFIX is always set. It might work if you erased it with something like cmake -DCMAKE_INSTALL_PREFIX= -DSimTK_INSTALL_PREFIX=${PREFIX}

Still, I'm having trouble thinking of any reason why we need to continue supporting SimTK_INSTALL_PREFIX.

Fix the use of SimTK_INSTALL_PREFIX for backward compatibility.
@scpeters
Copy link
Member Author

scpeters commented Oct 2, 2013

I just pushed a fix for the old-style -DSimTK_INSTALL_PREFIX option if CMAKE_INSTALL_PREFIX isn't set.

@chrisdembia
Copy link
Member

A) As is, the description for CMAKE_INSTALL_PREFIX is either "Install path prefix." or "Install directory." Could this be made consistent?

B) By default, I did not have a SimTK_INSTALL_DIR variable, and CMAKE_INSTALL_PREFIX was set to /usr/local. What is the desired behavior?

C) The backwards compatibility section checks to see if CMAKE_INSTALL_PREFIX is set or not; doesn't CMake set this by default? If so, this would always be false.

I didn't try actually using the library in its installed form; I just made sure there were no issues when running $ make install. I also built static libraries, and made sure those were installed. There were no issues doing so.

EDIT: I did my build/install before @scpeters added 7023a2b to the PR.

@sherm1
Copy link
Member

sherm1 commented Oct 2, 2013

No, I didn’t change anything there. It is attempting to turn off certain gcc optimizations that were buggy in gcc 4.4.3. But I see that the check is being done without checking whether the compiler is gcc or clang. What it calls GCC_VERSION might actually be clang version and then the version check doesn’t make sense. Probably it should repeat the if(${cmake_c_compiler} matches “cc”) test.

@sherm1 sherm1 merged commit ca24c54 into simbody:simbody-3.3 Oct 3, 2013
@sherm1
Copy link
Member

sherm1 commented Oct 3, 2013

I also pushed this to the master branch.

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.

None yet

3 participants