Review: Visual Studio 2010 compile fix Edit


Jeremy Selan <jeremy...@...>
 

http://github.com/imageworks/OpenColorIO/pull/300

Frederik has submitted a bunch of changes related to Visual Studio
2010 compatibility and STATIC linking.

As this touches CMake mojo as used on linux / osx, would those
familiar with package management please check this request out?

Thanks!

-- Jeremy

--------------------------------------------------------------------------------------------------------------

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:
http://stackoverflow.com/questions/1242820/can-a-c-static-library-link-to-shared-library
http://stackoverflow.com/questions/2157629/linking-static-libraries-to-other-static-libraries
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.


Richard Shaw <hobbe...@...>
 

On Fri, Dec 14, 2012 at 8:37 PM, Jeremy Selan <jeremy...@gmail.com> wrote:
http://github.com/imageworks/OpenColorIO/pull/300

Frederik has submitted a bunch of changes related to Visual Studio
2010 compatibility and STATIC linking.

As this touches CMake mojo as used on linux / osx, would those
familiar with package management please check this request out?
My github-fu isn't good enough. How would I go about getting a patch
from the pull request so I can test it?

Thanks,
Richard


dbr/Ben <dbr....@...>
 

On 18/12/2012, at 9:00 AM, Richard Shaw wrote:
My github-fu isn't good enough. How would I go about getting a patch
from the pull request so I can test it?

Add the pull-request-creator's repo as a remote, and fetch the changes

    $ git remote add frederik git://github.com/frederikaalund/OpenColorIO.git
    $ git fetch frederik

Then either check out the branch temporarily (in "detatched HEAD mode"):

    $ git checkout frederik/visual-studio-2010-compile-fix

Or as a tracking branch, which makes a new local branch connected to the remote:

    $ git checkout -t frederik/visual-studio-2010-compile-fix


Or, if you want an actual ye ol' patch, you can append ".patch" to the Github URL:


Richard Shaw <hobbe...@...>
 

On Tue, Dec 18, 2012 at 2:57 AM, dbr/Ben <dbr....@gmail.com> wrote:
On 18/12/2012, at 9:00 AM, Richard Shaw wrote:

My github-fu isn't good enough. How would I go about getting a patch
from the pull request so I can test it?


Add the pull-request-creator's repo as a remote, and fetch the changes

$ git remote add frederik
git://github.com/frederikaalund/OpenColorIO.git
$ git fetch frederik

Then either check out the branch temporarily (in "detatched HEAD mode"):

$ git checkout frederik/visual-studio-2010-compile-fix

Or as a tracking branch, which makes a new local branch connected to the
remote:

$ git checkout -t frederik/visual-studio-2010-compile-fix


Or, if you want an actual ye ol' patch, you can append ".patch" to the
Github URL:

https://github.com/imageworks/OpenColorIO/pull/300.patch
That's what I was looking for! I remembered you could do something
like that but I was trying .../pull/300/patch not .patch.

Thanks!

Richard


Richard Shaw <hobbe...@...>
 

On Fri, Dec 14, 2012 at 8:37 PM, Jeremy Selan <jeremy...@gmail.com> wrote:
http://github.com/imageworks/OpenColorIO/pull/300

Frederik has submitted a bunch of changes related to Visual Studio
2010 compatibility and STATIC linking.

As this touches CMake mojo as used on linux / osx, would those
familiar with package management please check this request out?
Ok, first problem. When I apply the patch to 1.0.8 I get the following
on my Linux build:

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....

Richard


Richard Shaw <hobbe...@...>
 

I'm not sure if this is causing any issues:

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


Is it a problem to include the same header twice? I'm not sure what we
should be doing differently for clang.

Richard


Richard Shaw <hobbe...@...>
 

On Tue, Dec 18, 2012 at 8:20 AM, Richard Shaw <hobbe...@gmail.com> wrote:
I'm not sure if this is causing any issues:

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 can confirm removing this commit fixes compilation on linux. I
haven't done any detailed analysis to see if there's any other
differences in the resultant package though.

Richard


Richard Shaw <hobbe...@...>
 

Ok, comprehensive package analysis:

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

I don't see anything that makes me believe the changes aren't safe
(minus the previously mentioned problem). At least on linux...

Richard