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

Exporting tensorflow package for cmake #14554

Merged
merged 2 commits into from
Nov 17, 2017
Merged

Exporting tensorflow package for cmake #14554

merged 2 commits into from
Nov 17, 2017

Conversation

PinkySan
Copy link
Contributor

This PR is based on the PR #13867.

It provides the usage of cmake find_package(). Therefore it should be easier to intergrate the prebuilt tensorflow code into existing external projects.

  1. What about a version control mechanism? (Cons: Has to be manually changed in every new version)
  2. Supplying some examples with the installed version (This examples should not be prebuilt, furthermore they should be built with an installed tensorflow)

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@PinkySan PinkySan changed the title exporting tensorflow package for cmake Exporting tensorflow package for cmake Nov 14, 2017
@jhseu jhseu requested a review from mrry November 16, 2017 20:52
@jhseu jhseu self-assigned this Nov 16, 2017
install(TARGETS tensorflow
target_include_directories(tensorflow PUBLIC
$<INSTALL_INTERFACE:include/>
$<INSTALL_INTERFACE:include/external/nsync/public>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the nsync headers missing a corresponding install(...) statement somewhere below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The headers are needed within core/platform/default/mutex.h, which will be installed.
The nsync headers are installed, but the tensorflow-target needs the ${install_prefix}/include/external/nsync/public as a target include directories.

We can change the path in the mutex.h file or adding the installed folder to the target within install_interface

Copy link
Contributor

@mrry mrry Nov 16, 2017

Choose a reason for hiding this comment

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

Ack, leaving it as is sounds good (I'm not particularly clear on how CMake installation works wrt. external packages). If it works for you, SGTM!

Copy link
Contributor Author

@PinkySan PinkySan Nov 16, 2017

Choose a reason for hiding this comment

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

This might not be the only target dependent installation property. Maybe the exported libraries will also need some target compile definitions. But these can easily be added

@jhseu jhseu added the kokoro:force-run Tests on submitted change label Nov 16, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 16, 2017
@jhseu jhseu added kokoro:force-run Tests on submitted change and removed kokoro:force-run Tests on submitted change labels Nov 16, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 16, 2017
@jhseu jhseu added the kokoro:force-run Tests on submitted change label Nov 17, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 17, 2017
@jhseu jhseu merged commit b70aa4d into tensorflow:master Nov 17, 2017
k-w-w pushed a commit that referenced this pull request Nov 17, 2017
* Exporting Targets

* reverting changes within tf_core_framework.cmake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants