Skip to content

CMake: Allow linking draco statically if ASSIMP_BUILD_DRACO_STATIC is set. - #5535

Merged
kimkulling merged 2 commits into
assimp:masterfrom
alexrp:master
Apr 8, 2024
Merged

CMake: Allow linking draco statically if ASSIMP_BUILD_DRACO_STATIC is set.#5535
kimkulling merged 2 commits into
assimp:masterfrom
alexrp:master

Conversation

@alexrp

@alexrp alexrp commented Apr 8, 2024

Copy link
Copy Markdown
Contributor

In Silk.NET we're packaging Assimp as a native dependency. We'd like to enable Draco in our Assimp build but would also like to avoid having to ship the extra shared library. This patch adds an option ASSIMP_BUILD_DRACO_STATIC which allows building and linking Draco statically into Assimp.

This only works when using the bundled Draco since there doesn't seem to be an obvious way to make it work with find_package().

I'm only able to test this on Linux. Hopefully it just works on Windows and macOS in CI...

@alexrp alexrp mentioned this pull request Apr 8, 2024
Open
kimkulling
kimkulling approved these changes Apr 8, 2024
View reviewed changes

@kimkulling kimkulling left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks fine.

@kimkulling
kimkulling merged commit b8aa68a into assimp:master Apr 8, 2024
@kimkulling

kimkulling commented Apr 8, 2024

Copy link
Copy Markdown
Member

Merged, thanks a lot for your contribution.

@Brakebein

Brakebein commented Jun 20, 2024

Copy link
Copy Markdown

FYI, it's a little breaking change! So far, I was building with

-DBUILD_SHARED_LIBS=OFF -DASSIMP_BUILD_DRACO=ON

After upgrading to v5.4.1 this build config was failing:

CMake Error at CMakeLists.txt:698 (set_target_properties):
  set_target_properties Can not find target to add properties to:
  draco_shared

CMake Error at CMakeLists.txt:379 (set_target_properties):
  set_target_properties Can not find target to add properties to:
  draco_shared
Call Stack (most recent call first):
  CMakeLists.txt:703 (TARGET_USE_COMMON_OUTPUT_DIRECTORY)

CMake Error at CMakeLists.txt:711 (INSTALL):
  INSTALL TARGETS given target "draco_shared" which does not exist.

I needed to explicitly set ASSIMP_BUILD_DRACO_STATIC to fix it.

-DBUILD_SHARED_LIBS=OFF -DASSIMP_BUILD_DRACO=ON -DASSIMP_BUILD_DRACO_STATIC=ON

@BrewTestBot BrewTestBot mentioned this pull request Jul 3, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Aug 30, 2024
Merged
kimkulling pushed a commit that referenced this pull request Jul 1, 2026
@alexrp @kimkulling
CMake: Allow linking draco statically if ASSIMP_BUILD_DRACO_STATIC is…
a0bafd5 
… set. (#5535)

* CMake: Mark draco as a private dependency since assimp does not publicly expose its interface.

* CMake: Allow linking draco statically if ASSIMP_BUILD_DRACO_STATIC is set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kimkulling kimkulling kimkulling approved these changes

Assignees

No one assigned

Labels

None yet

Projects

The Asset-Importer-Lib DevOps Board
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexrp @kimkulling @Brakebein