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

Make Draco packaging friendly when DRACO_TRANSCODER_SUPPORTED is enabled. #782

Closed
jansol opened this issue Dec 10, 2021 · 9 comments
Closed

Comments

@jansol
Copy link

jansol commented Dec 10, 2021

There is no bin/draco_transcoder in the install directory even if the DRACO_TRANSCODER_SUPPORTED option is enabled. Looking at draco_install.cmake this seems to be a simple oversight.

@tomfinegan
Copy link
Contributor

This is a known issue that needs a lot more than simple install of the binary.

For now I strongly suggest against installing the transcoder at a packaging level. Enabling the transcoder results in a libdraco with external dependencies that are not easily resolved at present.

  • TinyGLTF is not packaging friendly at present.
  • Eigen must be installed.
  • gulrak/filesystem is also likely required.

I'm not certain what approach you're taking as a packager, but "enable it all and let's see what happens" is not an approach or philosophy that I would recommend.

@tomfinegan
Copy link
Contributor

To follow up: downstream installation of a statically linked draco_transcoder should be fine, but anything further I would recommend against at present.

@tomfinegan
Copy link
Contributor

Oops- misclick. Coffee deprivation.

@tomfinegan tomfinegan reopened this Dec 10, 2021
@tomfinegan tomfinegan changed the title draco_transcoder does not get installed Make Draco packaging friendly when DRACO_TRANSCODER_SUPPORTED is enabled. Dec 10, 2021
@tomfinegan
Copy link
Contributor

Abusing your bug for tracking this. Sorry not sorry. :)

@jansol
Copy link
Author

jansol commented Dec 10, 2021

No problem, the transcoder just seemed like a sufficiently standalone CLI utility that I expected it to get installed along the encoder and decoder.

As for the dependencies, git submodules are well supported by Nix so for me they are not a problem. For official releases the usual procedure I've seen is that the maintainers upload a custom zip/tarball that has the submodules downloaded into it. Even nicer would be if there was a Find*.cmake for them so existing packages could be reused, but then again they are mostly small dependencies and at least Eigen is header-only IIRC so the gains from that are questionable.

Also I think the default compiler in nixpkgs supports the C++17 filesystem so if it's possible to detect that and make the gulrak/filesystem dependency optional that would be perfect. I'm not familiar with that stuff but I think it would be doable with a small test program, much like cmake tests for compiler support for other things.

@tomfinegan
Copy link
Contributor

Working from source and building from a clone with the submodules included isn't a problem at all, and if your packaging setup involves use of the git repository directly then you're probably fine already. In that case you are correct that lack of draco_transcoder in the install target is a simple oversight.

Before going over some of the details a note about my TinyGLTF comment: I was actually mistaken-- TinyGLTF isn't that packaging unfriendly, at least not enough that it's a real blocker. It just drops everything in CMAKE_INSTALL_FULL_INCLUDEDIR. Messy in my opinion, but basically harmless.

The main issue that has to be dealt with is disagreement between where Eigen and TinyGLTF live in relation to the Draco sources. In a development clone of Draco we have no issues, but when the packages Draco depends on are installed the includes within the Draco sources must change. For example, we have this now:

#include "third_party/tinygltf/tiny_gltf.h"

That works fine in a development tree. However, once installed, including TinyGLTF changes to:

#include "tiny_gltf.h"

Eigen has a similar issue, with a dev tree:

#include "Eigen/Geometry" or #include "third_party/eigen/Eigen/Geometry"

Installed:

The first form simply requires updated include paths passed to the compiler, but the second requires include changes at the source level: #include "eigen3/Eigen/Geometry"

I think filesystem is already ok since the source level includes already work for both the dev tree location and the installed location.

Anyway, this requires changes to our snapshotting/release procedure to address. Because of that the work will need to be handled upstream and then included in a future release, and unfortunately I can't provide any sort of eta at present.

@jansol
Copy link
Author

jansol commented Dec 10, 2021

TinyGLTF actually appears to have a CMake option to not install anything, so vendoring that is no problem if that option is simply disabled.

As I said, all of the dependencies seem to be header-only? In which case there is not much value in using distro-provided packages since they don't really have time-consuming compilation steps whose results could be reused across packages.

@tomfinegan
Copy link
Contributor

As I said, all of the dependencies seem to be header-only?

All dependencies are header only, but that doesn't really matter in this situation. As things are at present building an application that compiles/links against an installed draco library will be broken when draco is installed with DRACO_TRANSCODER_SUPPORTED enabled. This is because all draco .h files are installed, and some of those files include files that are part of Eigen and TinyGLTF. The paths in those include directives are not correct for usage with installed Eigen or TinyGLTF, so compiling files that include them is going to result in file not found errors.

None of this matters if the application using draco is doing so via direct usage of the CMake build, but installing draco and building against the installed library and its headers just won't work with DRACO_TRANSCODER_SUPPORTED enabled without resolving the includes issue as things are at present. This doesn't hurt anyone that just wants to run the draco tools, but developers won't be able to use the library or its includes without running into errors.

@tomfinegan
Copy link
Contributor

I think #812 fixes this. Closing this one-- if this remains an issue please ping this issue or open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants