Homebrew / legacy-homebrew

💀 The former home of Homebrew/homebrew (deprecated)
https://brew.sh
26.97k stars 11.34k forks source link

Unable to install open-scene-graph: missing dae.h for collada-dom #43536

Closed nextstopsun closed 8 years ago

nextstopsun commented 9 years ago

Trying to install open-scene-graph with: brew install --with-collada-dom --with-docs --with-ffmpeg --with-gdal --with-jasper --with-librsvg --with-qt open-scene-graph

Make fails on [ 95%] Built target osgdb_ive make: *** [all] Error 2

My brew logs are here: https://gist.github.com/b55823b91e605648ccfc

dunn commented 9 years ago

Looks like this is causing the failure:

In file included from /tmp/open-scene-graph20150903-1074-ipfyil/OpenSceneGraph-3.4.0/src/osgPlugins/dae/daeReader.cpp:14:
/tmp/open-scene-graph20150903-1074-ipfyil/OpenSceneGraph-3.4.0/src/osgPlugins/dae/daeReader.h:19:10: fatal error: 'dae.h' file not found
#include <dae.h>
         ^
nextstopsun commented 9 years ago

dae.h is a header file from collada-dom library I'm using brewed collada dom:

brew info collada-dom
collada-dom: stable 2.4.0
C++ library for loading and saving COLLADA data
http://www.collada.org/mediawiki/index.php/Portal:COLLADA_DOM
/usr/local/Cellar/collada-dom/2.4.0 (462 files, 13M) *
  Built from source
From: https://github.com/Homebrew/homebrew/blob/master/Library/Formula/collada-dom.rb

So I have this file in my filesystem

locate dae.h
/usr/local/Cellar/collada-dom/2.4.0/include/collada-dom2.4/dae.h

Maybe it should symlink to /usr/local/include/ or smth?

dunn commented 9 years ago

There should be a flag we can pass to CMake to let it know where to find that header; if you want to poke around that'd be great, otherwise I'll give it a shot when I have a chance.

nextstopsun commented 9 years ago

Hmm... I thought this line in the formula is passing the correct flag:

if build.with? "collada-dom"
      args << "-DCOLLADA_INCLUDE_DIR=#{Formula["collada-dom"].opt_include}/collada-dom"
    end

@dunn Can you give me a clue?

dunn commented 9 years ago

Oh, the problem is the include directory is named "collada-dom2.4" instead of "collada-dom". I guess we'll have to change that manually for each minor version bump.

nextstopsun commented 9 years ago

Thanks, @dunn hardcoding collada-dom2.4 in formula did the trick. But is there a proper way to point out to installed version of the lib?

DomT4 commented 9 years ago

Was this resolved?

dunn commented 9 years ago

Woops, I guess not.

One way to avoid hardcoding the collada version number into the open-scene-graph formula would be to do something silly like:

if build.with? "collada-dom"
  collada_dir = `ls #{Formula["collada-dom"].opt_include}/collada-dom*`
  args << "-DCOLLADA_INCLUDE_DIR=#{collada_dir}

I don't know which is worse.

apjanke commented 8 years ago

This sounds like an upstream bug in OSG's build process.

By default, collada-dom installs its headers in major.minor versioned directories. The OpenSceneGraph build system has custom code in CMakeModules/FindCollada.cmake to locate the Collada headers. It doesn't look for versioned directories. And the #include directives in its code don't qualify the file names.

FIND_PATH(COLLADA_INCLUDE_DIR dae.h
    ${COLLADA_DOM_ROOT}/include
    $ENV{COLLADA_DIR}/include
    $ENV{COLLADA_DIR}
    ~/Library/Frameworks
    /Library/Frameworks
    /opt/local/Library/Frameworks #macports
    /usr/local/include
    /usr/local/include/colladadom
    /usr/local/include/collada-dom
    /opt/local/include/collada-dom
    /usr/include/
    /usr/include/colladadom
    /usr/include/collada-dom
    /sw/include # Fink
    /opt/local/include # DarwinPorts
    /opt/csw/include # Blastwave
    /opt/include
    /usr/freeware/include
    ${ACTUAL_3DPARTY_DIR}/include
)

So, it looks like OpenSceneGraph can't compile against a default installation of collada-dom; it expects a custom one that has removed the version from the directory.

It's kind of unusual for software to qualify its include dirs by minor version by default. And since OSG is the only formula in Homebrew using collada-dom, I don't know how software usually handles it. Though having to change your code on every minor version bump is really inconvenient, so I suspect they do something like this where they don't qualify the header paths in their #defines, and just locate the Collada-dom installation's include dir to put on the -I path.

We could change the collada-dom installation to not have a version-qualified installation directory. Or to symlink a include/collada-dom -> collada-dom2.4 during the installation process. If OSG's CMake stuff is right, that's what DarwinPorts and Fink did. Then OSG's default build logic would work.

Not sure what the official Right Way to do it is, or if there even is one. The Collada doco on the OSG Wiki is out of date, and the library client guide on the Collada DOM Wiki also looks out of date, and doesn't match how OSG is using it.

I'm inclined to just include an unqualified include/collada-dom link or rename in the collada-dom formula, since we don't support side-by-side installation of multiple versions, and it seems to be what the client program wants.

apjanke commented 8 years ago

On second thought, @dunn's hack seems like a better approach, since it doesn't require any patching or change collada-dom's behavior away from the upstream default. Since we only have one formula using collada-dom, and I couldn't find any other such clients with a little googling, seems safest to restrict the change to the one failing client.

Made a PR for it: #46380. Tests out okay for me on 10.9.5 doing brew install --build-from-source open-scene-graph --with-collada-dom.

apjanke commented 8 years ago

Oops. The test I ran for #46380 was wrong. Now it is failing with this.

[ 87%] Building CXX object src/osgPlugins/dae/CMakeFiles/osgdb_dae.dir/daeReader.cpp.o
cd /tmp/open-scene-graph20151126-78498-1ygkfdp/OpenSceneGraph-3.4.0/build/src/osgPlugins/dae && /usr/local/Library/ENV/4.3/clang++   -DCOLLADA_DOM_SUPPORT141 -DNO_BOOST -Dosgdb_dae_EXPORTS -I/tmp/open-scene-graph20151126-78498-1ygkfdp/OpenSceneGraph-3.4.0/include -F/Applications/Xcode-6.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/System/Library/Frameworks -I/tmp/open-scene-graph20151126-78498-1ygkfdp/OpenSceneGraph-3.4.0/build/include -I/usr/local/opt/collada-dom/include/collada-dom2.4 -I/1.4  -std=c++11 -stdlib=libc++  -Wno-conversion -arch x86_64 -isysroot /Applications/Xcode-6.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk -fPIC   -o CMakeFiles/osgdb_dae.dir/daeReader.cpp.o -c /tmp/open-scene-graph20151126-78498-1ygkfdp/OpenSceneGraph-3.4.0/src/osgPlugins/dae/daeReader.cpp
In file included from /tmp/open-scene-graph20151126-78498-1ygkfdp/OpenSceneGraph-3.4.0/src/osgPlugins/dae/daeReader.cpp:14:
/tmp/open-scene-graph20151126-78498-1ygkfdp/OpenSceneGraph-3.4.0/src/osgPlugins/dae/daeReader.h:22:10: fatal error: 'dom/domCommon_color_or_texture_type.h' file not found
#include <dom/domCommon_color_or_texture_type.h>
         ^

It looks like this is due to it not picking up the 1.4 subdirectory (for DOM v 1.4) under include/collada-dom2.4. That -I/usr/local/opt/collada-dom/include/collada-dom2.4 -I/1.4 should be -I/usr/local/opt/collada-dom/include/collada-dom2.4 -I/usr/local/opt/collada-dom/include/collada-dom2.4/1.4.

apjanke commented 8 years ago

I submitted an upstream issue: openscenegraph/osg#48.

apjanke commented 8 years ago

Hmm. Since this directory location is more a property of the collada-dom installation, might it make sense to add a special versioned_include_dir method to collada-dom.rb, which will tell you where this version-qualified directory ended up? Then it would be obvious when it needed to be adjusted if the collada-dom version was bumped, since it's in the same file.

MikeMcQuaid commented 8 years ago

Sounds like this was fixed in #46776. Otherwise: we'll accept PRs for this but we're not actively working on it at this time.