-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Remove libTensorFlow forced linking. #25222
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
Conversation
For reference, this is the test I ran (along with the normal tests)
|
@swift-ci please test tensorflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This may break some things I'm not able to test on mac, but I think we should fix-forward that.
I wonder what breakages do you anticipate on macOS? I believe changes in lib/Driver/UnixToolChains.cpp
don't affect macOS, which uses the related lib/Driver/DarwinToolChains.cpp
file.
Hi Dan, |
llvm::sys::path::append(swiftTensorFlowLibPath, "libswiftTensorFlow.so"); | ||
if (llvm::sys::fs::exists(swiftTensorFlowLibPath)) { | ||
Arguments.push_back("-lswiftTensorFlow"); | ||
Arguments.push_back("-ltensorflow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes from @pschuh for reference:
- The
link "tensorflow"
added tostdlib/public/CTensorFlow/module.modulemap
should eliminate the need forArguments.push_back("-ltensorflow");
here. -lswiftPython
and-lswiftTensorFlow
may be fixed by specifying the-module-link-name
flag.
@swift-ci please test tensorflow gpu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pschuh !
I seem to be able to run
swift test.swift
without these linked in, so I think that they're unnecessary. Also, fixed the "-ltensorflow" problem in a different way. This may break some things I'm not able to test on mac, but I think we should fix-forward that.