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

Dockerize FBX2glTF #184

Merged
merged 7 commits into from
May 8, 2019
Merged

Conversation

FreakTheMighty
Copy link
Contributor

@FreakTheMighty FreakTheMighty commented May 1, 2019

This pull request dockerizes FBX2glTF and is based on the azure build file. I've included a simple docker-compose.yaml because I find its syntax more convenient. A few notes

  • To build an image run docker-compose build.
  • To run in the container run docker-compose run fbx2gltf bash. FBX2glTF is on the PATH, so it can be executed from anywhere.
  • I attempted to use git lfs to fetch the FBX SDK, but it caused problems in managed build environments such as Docker Hub and Google Cloud Build. Instead, the Dockerfile fetches the SDK directly from Autodesk
  • If this pull request is accepted, it'd be great to setup automatic builds on Docker Hub.

I am still finding that the latest build segfaults, but that appears to match the current build on azure. Related #183

@zellski
Copy link
Contributor

zellski commented May 2, 2019

Many thanks for this work! Apologies for the simplistic question, though –– can you walk me through what Dockerising accomplishes in this case? The statically linked, precompiled binaries are meant to be pretty reliably executable in most environments.

Are you sure 'git lfs' can't fairly easily be installed in these managed environments? I'm a little concerned that there may be some confusing legal distinction between fetching and auto-installing SDKs from Autodesk, compared to a Git LFS fetch. Autodesk's license is not super clear (at least to me.)

pip install conan && \
conan remote add bincrafters https://api.bintray.com/conan/bincrafters/public-conan

COPY . /fbx2gltf
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this COPY be easily made to skip "./sdk/" by any chance? That's a lot of pointless data, if you're sure GIT LFS is not reliable everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should be able to add that to .dockerignore. The large copy wouldn't be an issue for docker hub builds because git lfs files haven't been pulled, but I can see this as useful for local builds.

@FreakTheMighty
Copy link
Contributor Author

@zellski no problem on the pull request! Also, those are all totally fair questions, I could be convinced the Dockerfile isn't work adding. Here's my thinking though. There are two main reasons I see for adding it.

  1. It provides a clear and reproducible record for how to build from source. It's similar to the azure job, but it can be run locally or remotely. This is helpful for development and testing.
  2. I would like to integrate FBX2glTF into a microservice, but ATM it looks like the binary releases and the npm directory are out of date. The azure job is a step in the right direction, but the artifacts don't feel like reliable things to base a docker image on. I think this is the weaker of the two reasons, as coming up with better binary distribution would make adding this executable to a docker service

Regarding git lfs and the fbx license. I feel confident that git lfs doesn't work on docker hub or google cloud build. Here is a build from docker hub demonstrating the problem and an issue requesting support.

I can't speak to the legality of this approach. It feels to me that coping the binary and redistributing it via git lfs is equivalent to what's done in the Dockerfile, but I really don't know.

As long as we know precisely what we're running, there's no need
to trust in Conan's guesswork.

(This also uses 'docker-build' for a build directory, to reduce
risk of conflict with the local repo checkout.)
@zellski
Copy link
Contributor

zellski commented May 7, 2019

Yeah, after a day or two of playing around with this, I remember now all the ways in which Docker helps.

I committed a "fix" to the segfault problem on master here, and pushed a PR to your PR with the necessary tweak to Dockerfile. If you can integrate that here, I think we're good to go.

@FreakTheMighty
Copy link
Contributor Author

Great, I’ll get that integrated tonight or tomorrow.

The Dockerfile can tell Conan exactly what its system is.
@zellski zellski merged commit 1100e09 into facebookincubator:master May 8, 2019
@zellski
Copy link
Contributor

zellski commented May 8, 2019

Thanks a lot!

@FreakTheMighty
Copy link
Contributor Author

I think it may have been premature to merge this. The last change to the dockerfile failed to build on docker hub. I'm looking into and will get back to you.

@zellski
Copy link
Contributor

zellski commented May 8, 2019

That's odd. The compiler settings should be guaranteed identical, given Docker magic, no? Ah well. Just do a follow-up PR. I wish I were someone who kept a tidy source control history, but it's... not my strength.

@FreakTheMighty
Copy link
Contributor Author

Yeah . . . not exactly sure what happened on that build. I suspect that the difference between local testing you did and the remote, was that your local ADD would have add sdk files pulled.
Docker cloud doesn't let me share urls for failed builds, even for public repos, but at the very end this happened:

Please set the environment variable FBXSDK_SDKS.
cp: cannot stat 'docker-build/FBX2glTF': No such file or directory
Removing intermediate container 62e9e42d840f
The command '/bin/sh -c conan install . -i docker-build -s build_type=Release -s compiler=gcc -s compiler.version=5 -s compiler.libcxx=libstdc++11 && conan build -bf docker-build . && cp docker-build/FBX2glTF /usr/bin && cd / && rm -rf /fbx2gltf /root/.conan' returned a non-zero code: 1

I'm adding the sdk to .dockerignore like you suggested. This has the added benefit of making it possible for me to swap the order of the FBX install and the source code COPY. That way, if you make a change to the source code it won't invalidate FBX SDK cache. This should speed up builds.

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

Successfully merging this pull request may close these issues.

3 participants