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

Visual Studio 2010, 2012, 2013, and "14" compile fix #300

Conversation

frederikaalund
Copy link

I changed the CMake files to fix the compilation issues I (and apparently others) had while trying to compile OpenColorIO on Windows 7 using Visual Studio 2010. This works for the STATIC version of OpenColorIO. I have not tried the SHARED version. Also, I only built the core OpenColorIO library and not any of the tools.

Notable changes:

  • Both tinyxml and yaml-cpp can now be found externally using the USE_EXTERNAL_* options in CMake. This is useful when ExternalProject_Add doesn't work as it should.
  • Removed the PkgConfig dependency for yaml-cpp by adding a very raw FindYAML_CPP.cmake package. The latter has a lot of room for improvement and is merely intended as a quick-fix. This is useful when PkqConfig is not present which is rarely the case with Windows Installations
  • The static build of OpenColorIO is no longer linked with external libraries. Static libraries do not incorporate the binary objects of its dependencies. It is up to the runtime binary that links against OpenColorIO to link the entire hierarchy of dependencies. In other words, the target_link_libraries has no effect on archive targets (ceated with add_library(TARGET_NAME STATIC ...)). Sources:
  • Added the definition OCIO_BUILD_STATIC to static builds so the code doesn't use __declspec(dllimport) when building statically. This fixes a lot of linker errors.

# target_link_libraries(OpenColorIO_STATIC
# debug ${EXTERNAL_DEBUG_LIBRARIES}
# optimized ${EXTERNAL_OPTIMIZED_LIBRARIES}
# general ${EXTERNAL_GENERAL_LIBRARIES})
else()
target_link_libraries(OpenColorIO_STATIC ${EXTERNAL_GENERAL_LIBRARIES})
Copy link
Author

Choose a reason for hiding this comment

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

I think this line should be removed too. See the comment in the pull request.

@dbr
Copy link
Contributor

dbr commented Dec 18, 2012

This breaks building on OS X for me, specifically the changes to OpenColorABI.h causes lots of linker errors:

$ cmake -D CMAKE_INSTALL_PREFIX=../dist -DOCIO_BUILD_TESTS=yes .. && make -j1
[...]
[ 89%] Building CXX object src/pyglue/CMakeFiles/PyOpenColorIO.dir/PyDisplayTransform.cpp.o
Linking CXX executable ociobakelut
Undefined symbols for architecture x86_64:
  "OpenColorIO::v1::Baker::getFormatNameByIndex(int)", referenced from:
      _main in main.cpp.o
  "OpenColorIO::v1::Baker::getFormatExtensionByIndex(int)", referenced from:
      _main in main.cpp.o
[...]
ld: symbol(s) not found for architecture x86_64
collect2: ld returned 1 exit status

@frederikaalund
Copy link
Author

dbr, thanks for the feedback! Your findings are both unfortunate and strange since it builds on OSX 10.8 for me. :/ I use clang with -std=c++11 and -stdlib=libc++ so maybe that is the difference. Anyways, I can't really see what the problem could be from the linker errors you posted. Are you sure they are related to the changes in OpenColorABI.h and not to the CMake files? I.e., does it compile and link with the old OpenColorABI.h?

If you are using clang you might want to try the last commit.

@dbr
Copy link
Contributor

dbr commented Dec 19, 2012

Are you sure they are related to the changes in OpenColorABI.h and not to the CMake files?

Quite right. It's a combination of the two (I was removing diff-chunks in reverse order, which was confused matters)

Specifically: add_definition(-DOCIO_BUILD_STATIC) will be applied to all targets, including the dynamic library, which is logically wrong

It's interesting it worked fine for you - maybe clang or the newer std* stuff is less dependant on the visibility attributes?

Regardless, the fix is easy - remove the add_definition line, and move the OCIO_BUILD_STATIC definition to the static target properties:

if(OCIO_BUILD_STATIC)

    [..snip..]

    set_target_properties(OpenColorIO_STATIC PROPERTIES
        OUTPUT_NAME OpenColorIO
        COMPILE_FLAGS "${EXTERNAL_COMPILE_FLAGS} -DOCIO_BUILD_STATIC"
        LINK_FLAGS "${EXTERNAL_LINK_FLAGS}")

    [..snip..]

With that changed, all builds on OS X perfectly for me

The static build of OpenColorIO is no longer linked with external libraries

Huh, that's a good point. Perhaps we should remove the target_link_libraries for non-WIN32 also? Although it seems harmless

Nitpick: should remove the commented out target_link_libraries call, or if it's still useful then add a comment explaining why it might be commented (otherwise it might be confusing)

@hobbes1069
Copy link
Contributor

I've been replying to the ocio-dev list instead of here so I'll cut-n-paste my findings for linux:

The following change is causing a problem for linux:

diff --git a/src/core/PathUtils.cpp b/src/core/PathUtils.cpp
index a04ecee..4c944b1 100644
--- a/src/core/PathUtils.cpp
+++ b/src/core/PathUtils.cpp
@@ -49,10 +49,14 @@
 #if defined(__APPLE__) && !defined(__IPHONE__)
 #include <crt_externs.h> // _NSGetEnviron()
 #elif !defined(WINDOWS)
-#include <unistd.h>
 extern char **environ;
 #endif

+#include <unistd.h>
+#if defined(__clang__)
+#include <unistd.h>
+#endif

I'm not exactly sure what the last part does except for include unistd.h twice if clang is defined. I get the following error building for Fedora x86_64 with this patch applied:

cd /home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/build/docs/setuptools-prefix/src/setuptools
&& /usr/bin/cmake -E touch
/home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/build/docs/setuptools-prefix/src/setuptools-stamp/setuptools-build
/usr/bin/cmake -E cmake_progress_report
/home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/build/CMakeFiles
/home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/src/core/PathUtils.cpp:52:15:
error: previous declaration of 'char** environ' with 'C++' linkage
In file included from
/home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/src/core/PathUtils.cpp:55:0:
/usr/include/unistd.h:546:15: error: conflicts with new declaration
with 'C' linkage
/home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/src/core/PathUtils.cpp:
In function 'std::string OpenColorIO::v1::pystring::os::getcwd()':
/home/build/rpmbuild/OpenColorIO/BUILD/OpenColorIO-1.0.8/src/core/PathUtils.cpp:125:33:
warning: ignoring return value of 'char* getcwd(char*, size_t)',
declared with attribute warn_unused_result [-Wunused-result]
make[2]: *** [src/core/CMakeFiles/OpenColorIO.dir/PathUtils.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....

But I get a good build with that one hunk removed. The following links compare the package and abi differences with and without the patch:

pkgdiff:
https://dl.dropbox.com/u/34775202/pkgdiff/ocio/changes_report.html

abi-compliance-checker:
https://dl.dropbox.com/u/34775202/pkgdiff/ocio/compat_report.html

	- Fixed when the OCIO_BUILD_STATIC flag gets defined so that OCIO also builds on OSX with a non-clang compiler.
	- I accidentally included <unistd.h> for all builds in PathUtils.cpp because of copy-paste errors. This should now be fixed.
@frederikaalund
Copy link
Author

Thanks for the input from both of you! :)

@dbr I've moved the OCIO_BUILD_STATIC flag like you suggest. Also, the commented-out lines have been removed along with the non-win32 target_link_libraries. Like you say it is harmless to use target_link_libraries on static library targets but it might confuse people (like me). It does have one good use: If you link the target (I.e., OpenColorIO_STATIC) to a runtime binary then the dependencies of the former are given to the latter. Since you don't do this, it should be harmless to remove the lines.

@hobbes1069 Nice find! It was a copy-paste error. ;) It should be fixed now so that <unistd.h> is only included when clang is defined.

I hope that is it but otherwise please report back!

@hobbes1069
Copy link
Contributor

That last update for the includes fixed it for me.

	- Fixed when the OCIO_BUILD_STATIC flag gets defined so that OCIO also builds on OSX with a non-clang compiler.
	- I accidentally included <unistd.h> for all builds in PathUtils.cpp because of copy-paste errors. This should now be fixed.
…alund/OpenColorIO into visual-studio-2010-compile-fix
@frederikaalund
Copy link
Author

James Burgess made me aware of the fact that Little CMS (lcms) wouldn't build in the default Windows environment. You can now use the option USE_EXTERNAL_LCMS to point CMake to an externally built lcms library. Notice that USE_EXTERNAL_LCMS was already present previously but didn't have any effect. I don't know why, I didn't put it there).
I've tested this by building the ociobakelut application statically using lcms2-2.4. Hopefully this should resolve that issue. If not, then please reply! :)

…ent dependencies (CMake policy CMP0046). Added a missing include directive. Added boost dependency to build configuration for yaml-cpp versions 5.0.0 and greater. OCIO now compiles using Visual Studio "14" CTP3.
@frederikaalund
Copy link
Author

This patch also fixes compilation for Visual Studio "14" CTP 3 on Windows 8.1 since the latest commit (acc3735). Consequently, compilation also works for Visual Studio 2013 (and even the Visual Studio 2013 November CTP).

@frederikaalund frederikaalund changed the title Visual Studio 2010 compile fix Visual Studio 2010, 2012, 2013, and "14" compile fix Sep 8, 2014
@Shootfast
Copy link
Contributor

Hi there,

I'm interested in applying any patches that improve support on windows, but could I ask you to rebase your changes onto the latest master and squash your commits down to give a relevant history (eg, no upstream pull, or merges)? Just so I can clearly see what changes we are applying, and perhaps avoid any breakages on other platforms.

Cheers,
Mark

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

4 participants